Mercury Bugs - mercury |
| View Issue Details |
|
| ID | Project | Category | View Status | Date Submitted | Last Update |
| 0000532 | mercury | Bug | public | 2021-05-12 15:52 | 2021-05-13 20:00 |
|
| Reporter | wangp | |
|---|
| Assigned To | zs | |
|---|
| Priority | normal | Severity | minor | Reproducibility | always |
|---|
| Status | resolved | Resolution | fixed | |
|---|
| Platform | | OS | | OS Version | |
|---|
| Product Version | | |
|---|
| Target Version | | Fixed in Version | | |
|---|
|
| Summary | 0000532: constant_prop_2 failure in low-level C grades |
|---|
| Description | The hard_coded/constant_prop_2.m test case fails in low-level C grades. When --loop-invariants is enabled (e.g. with -O5) the first test is not being optimised away:
( if "abc" ++ "xyz" = "abcxyz" then
io.write_string("yes", !IO), io.nl(!IO)
else
link_error(!IO)
).
I have bisected it to commit 18e222656 "Stop common_struct from interfering with mark_static_terms."
When --loop-invariants is enabled, the mark_static_terms.m pass is run, which marks the unification as construct_statically:
% constant_prop_2.hlds_dump.148-mark_static
constant_prop_2.main(STATE_VARIABLE_IO_0, STATE_VARIABLE_IO) :-
( if
( % conjunction
V_6 = "abcxyz"
% V_6 <= "abcxyz"
% cell_is_unique
% construct statically
,
V_6 = "abcxyz"
% V_6 ?= "abcxyz"
)
then
( % conjunction
V_9 = "yes"
% V_9 <= "yes"
% cell_is_unique
% construct statically
,
io.write_string(V_9, STATE_VARIABLE_IO_0, STATE_VARIABLE_IO_10)
,
io.nl(STATE_VARIABLE_IO_10, STATE_VARIABLE_IO)
)
else
constant_prop_2.link_error(STATE_VARIABLE_IO_0, STATE_VARIABLE_IO)
).
But commit 18e222656 disables the optimisation unless How = construct_dynamically.
NOTE: the comment in common_optimise_unification incorrectly states that
mark_static_terms.m is only run in MLDS grades. maybe_mark_static_terms is currently located in mercury_compile_mlds_back_end.m, but it is called by maybe_loop_inv. |
|---|
| Steps To Reproduce | % mmc -s asm_fast.gc --loop-invariants constant_prop_2.m
/usr/bin/ld: constant_prop_2.o: in function `<predicate 'main'/2 mode 0>':
constant_prop_2.c:(.text+0x59): undefined reference to `<predicate 'constant_prop_2.link_error'/2 mode 0>'
Whether you actually get a link error seemingly depends on your version of gcc, linker, and phase of the moon. |
|---|
| Tags | No tags attached. |
|---|
| Relationships | |
| Attached Files | |
|---|
|
Notes |
|
|
(0001135)
|
|
zs
|
|
2021-05-12 16:30
|
|
Two things.
First, I have started on a diff to move maybe_mark_static_terms
from mercury_compile_mlds_backend.m to mark_static_terms.m, which
is already in the backend-neutral hlds package.
Second, I can see two broad approaches for the restoration of
the status-quo-ante of that bisected commit. One is to modify
loop_inv, which screws things up for the LLDS backend by getting
mark_static_terms to tinker with the construct_how fields in ways
that the LLDS backend does not expect, by making it clean up its
mess, and put the original values back into those fields. This
works only if we know what those original values are. The other
way is to set a flag in the simplify_info that specifies whether
we are targeting a backend that wants to pay attention to
construct_statically values in construct_how fields, or not.
In the latter case, we get commom_optimise_unification to accept
How = construct_statically as well as construct_dynamically.
The second is much easier to program, but imposes a (tiny) cost
on all compilations. I think that cost is too small to worry about,
so I would prefer the second approach.
Opinions? |
|
|
|
(0001136)
|
|
wangp
|
|
2021-05-12 18:38
|
|
The second sounds okay. There could be a danger that someone tries to make use of construct_statically in the LLDS backend when that information is no longer valid, but that will have to be clarified through comments.
Or, would it be possible to rerun mark_static_terms.m to recompute the how_to_construct fields after common.m makes a change that might invalidate those fields (for the MLDS backend only)?
|
|
|
|
(0001137)
|
|
zs
|
|
2021-05-12 19:49
|
|
Since the LLDS backend's code for creating static data predates
mark_static_terms.m, it should not pay any attention to construct_statically.
You are right, that should be documented as part of fixing this bug.
I don't understand your second paragraph. I am proposing that the code
in common.m that may invalidate the construct_how fields for the MLDS backend
be run ONLY when targeting the LLDS backend, so the situation assumed by
your question wouldn't arise. |
|
|
|
(0001138)
|
|
wangp
|
|
2021-05-13 12:10
|
|
> I don't understand your second paragraph. I am proposing that the code
> in common.m that may invalidate the construct_how fields for the MLDS backend
> be run ONLY when targeting the LLDS backend, so the situation assumed by
> your question wouldn't arise.
Sorry, here is the context:
I am considering what if we allow common.m to optimise away
constructions marked 'construct_statically',
i.e. the situation before commit 18e222656.
That would obviously reintroduce bug 0000493 for the MLDS backend,
because a construction that is optimised away may invalidate a
'construct_statically' annotation on another construction
in the same procedure.
I think we can fix that by making common.m re-run the mark_static_terms
algorithm on a procedure if it optimises away a construction marked
'construct_statically'.
We would need to change mark_static_terms so that it can change
constructions from 'construct_statically' back to 'construct_dynamically'
if a variable on the RHS of the construction is NOT in the static set. |
|
|
|
(0001139)
|
|
zs
|
|
2021-05-13 12:27
|
|
I think we are still talking at cross purposes. I am proposing
a change that would not affect what we do in MLDS grades at all.
(I have actually implemented it, the bootcheck is going right now.)
It affects only what we do in LLDS grades. Since LLDS grades
do not pay attention to construct_statically (treating it the same
as construct_dynamically), they do not CARE whether or not the invariants
established by mark_static_terms.m are preserved by common.m. |
|
|
|
(0001140)
|
|
zs
|
|
2021-05-13 20:00
|
|
|
Fix committed 20201 may 13. |
|