Location checks in ResolutionContext::lookup complicate replacing/moving IR nodes

[grg]

The Tofino code replaces/moves nodes in several places, such as during V1 -> TNA translation. During this translation, the compiler moves code from the VerifyChecksum block into the Parser block, translating verify_checksum into Checksum.add calls.

ResolutionContext::lookup compares the location of the name being looked up with the potential declaration to determine whether the declaration is a valid match. It will only match if the declaration is before the name, and it uses the SourceInfo to perform that comparison:

                Util::SourceInfo nsi = name.srcInfo;
                Util::SourceInfo dsi = d->getNode()->srcInfo;
                bool before = dsi <= nsi;

(See here and here.)

The issue is that after moving/replacing IR nodes, the source info may no longer reflect the IR order and so the comparison will produce the wrong result.

Consider a program as follows:

 1: control ComputeChecksum(...) {
 2:     apply {
 3:         verify_checksum(hdr.ipv4.isValid(),
 4:             {
 5:                 hdr.ipv4.version,
 6:                 ...
 7:             },
 8:             ...
 9:         );
10:     }
11: }
12:
13: parser IngressParser(..., out headers_t hdr, ...) {
14:     ...
15: }

When performing the V1 -> TNA translation, the verifyChecksum code from ComputeChecksum is moved and translated into IngressParser.

The challenge is what to use for the SourceInfo for the Checksum.add() calls inserted in IngressParser, such as checksum_0.add(hdr.ipv4.version)? A reasonable choice would be to use L5 as the SourceInfo for hdr.ipv4.version Member node in the add call. However during name lookup, the name SourceInfo would be L5, and the declaration SourceInfo would be L13, and so the before check would fail. The other choice then is to use a SourceInfo of the parser instance or beyond, but then error reports will not accurately reflect the location of hdr.ipv4.version in the input file.

This because an issue for the Tofino project when TypeInference switched from using a ReferenceMap object and instead relying on ResolutionContext.

[grg]

My current thinking is that instead of using the SourceInfo to check the ordering of the name vs declaration, the compiler should be looking at their relative locations within the IR tree.

[asl]

@grg I was already bitten by this couple of times as well. I tend to agree that source info should not be used here. Checking declared-before seems to be pretty straightforward for tree-like IR. We need to be more careful with DAGs though.

PS: Actually, the entire ResolutionContext / ReferenceMap should not be used anywhere, but this is a separate issue :)

[grg]

@asl How did you resolve it when you were bitten by it? I'm manipulating the source info currently, but it feels icky.

[asl]

@asl How did you resolve it when you were bitten by it? I'm manipulating the source info currently, but it feels icky.

Sadly, but this was the only workaround I ended with...

[asl]

Just in case, the same issue could be reproduced in p4c mainline if one would port MoveActionToTables / SynthesizeActions to ResolutionContext.