0000532: constant_prop_2 failure in low-level C grades

View Issue Details [ Jump to Notes ]

[ Issue History ] [ Print ]

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

Priority

normal

Severity

minor

Reproducibility

always

Status

resolved

Resolution

fixed

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.

Attached Files

Relationships

Relationships

Notes
~0001135 zs (developer) 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 (developer) 2021-05-12 18:38 Last edited: 2021-05-12 18:38 View 2 revisions	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 (developer) 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 (developer) 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 (developer) 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 (developer) 2021-05-13 20:00	Fix committed 20201 may 13.

Notes

Date Modified	Username	Field	Change
Issue History
2021-05-12 15:52	wangp	New Issue
2021-05-12 16:30	zs	Note Added: 0001135
2021-05-12 18:38	wangp	Note Added: 0001136
2021-05-12 18:38	wangp	Note Edited: 0001136	View Revisions
2021-05-12 19:49	zs	Note Added: 0001137
2021-05-13 12:10	wangp	Note Added: 0001138
2021-05-13 12:27	zs	Note Added: 0001139
2021-05-13 20:00	zs	Assigned To	=> zs
2021-05-13 20:00	zs	Status	new => resolved
2021-05-13 20:00	zs	Resolution	open => fixed
2021-05-13 20:00	zs	Note Added: 0001140

Issue History