Mercury Bugs - mercury
View Issue Details
0000532mercuryBugpublic2021-05-12 15:522021-05-13 20:00
Reporterwangp 
Assigned Tozs 
PrioritynormalSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version 
Target VersionFixed in Version 
Summary0000532: constant_prop_2 failure in low-level C grades
DescriptionThe 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.
TagsNo tags attached.
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.

Issue History
2021-05-12 15:52wangpNew Issue
2021-05-12 16:30zsNote Added: 0001135
2021-05-12 18:38wangpNote Added: 0001136
2021-05-12 18:38wangpNote Edited: 0001136bug_revision_view_page.php?bugnote_id=1136#r56
2021-05-12 19:49zsNote Added: 0001137
2021-05-13 12:10wangpNote Added: 0001138
2021-05-13 12:27zsNote Added: 0001139
2021-05-13 20:00zsAssigned To => zs
2021-05-13 20:00zsStatusnew => resolved
2021-05-13 20:00zsResolutionopen => fixed
2021-05-13 20:00zsNote Added: 0001140