Source info does not get propagated during inlining

[kfcripps]

Building the following program:

control C(out bit<64> v64) {
    action a2(in bit<1> value) {
        v64 = (bit<64>)(int<64>)(int<1>)value;
    }

    action a() {
        a2(1);
    }

    table t {
        actions = { a; }
        default_action = a;
    }

    apply {
        t.apply();
    }
}

control proto(out bit<64> v64);
package top(proto p);

top(C()) main;

results in (note no source info):

[--Wwarn=overflow] warning: 1s1: signed value does not fit in 1 bits
[--Wwarn=mismatch] warning: -64w1: negative value with unsigned type

After the first LocalCopyPropagation pass, the IR looks like:

    @name("C.a") action a() {
        v64 = (bit<64>)(int<64>)(int<1>)1w1;
    }

and the ConstantFolding pass emits the warning.

I also noticed that, in the following IR generated by the ActionsInliner pass:

    @name("a2") action a2_0(@name("value") in bit<1> value_0) {
        v64 = (bit<64>)(int<64>)(int<1>)value_0;
    }
    @name("a") action a_0() {
        @inlinedFrom("a2") {
            @name("value") bit<1> value = 1w1;
            v64 = (bit<64>)(int<64>)(int<1>)value;
        }
    }

The PathExpression value in (bit<64>)(int<64>)(int<1>)value has no source info.

@ChrisDodd @asl Should ActionsInliner be propagating source info here?

[kfcripps]

Another question is, should this warning be emitted at all? Section 8.11.1 of the spec mentions compiler warnings for int -> bit<w> and int -> int<w> casts, but not for casts from bit<m> -> int<n>

[ChrisDodd]

bit<m> -> int<n> (with m != n) should be an error. The warnings here are about constants that are out-of-range for the type(s) they are cast to. Given that these are explicit casts, it is not clear whether they should be warnings or not. There should definitely be a warning for implicit conversions (int -> int<n> or int -> bit<n>) when the constant is out of range.

[kfcripps]

bit -> int (with m != n) should be an error.

Sorry, I meant to say bit<n> -> int<n>

[ChrisDodd]

Irrespective of whether these should be warnings or not, ActionsInliner should be propagating the SourceInfo so that correct warnings for things could be made.

[ChrisDodd]

In general, IMO there should NOT be warnings for legal explicit casts -- often times the point of an explicit cast is to silence the warning from an implicit cast.

[kfcripps]

Irrespective of whether these should be warnings or not, ActionsInliner should be propagating the SourceInfo so that correct warnings for things could be made.

I think that argument->srcInfo needs to be propagated to the newly created PathExpression here (and maybe also in one or two other places in this file).

        if (param->direction == IR::Direction::In || param->direction == IR::Direction::InOut) {
            const auto *vardecl = new IR::Declaration_Variable(
                argument->srcInfo, newName, param->annotations, param->type, argument->expression);
            body.push_back(vardecl);
            subst.add(param, new IR::Argument(argument->name, new IR::PathExpression(newName)));

@ChrisDodd If you agree, I can open a PR to do this.

[ChrisDodd]

Yes, though the srcInfo that should be used is the srcInfo from the existing PathExpression being cloned/visited in SubstituteParameters when it does the substitution, and not the srcInfo from the argument. Not sure exactly where that happens, as SubstituteParameters seems to be cloning things when it doesn't need to and not cloning things when it does need to, so I'm not completely sure how it ends up working.