[SelectionDAG] Assertion due to MachineMemOperand flags difference.

Hello,

I've hit an assertion in SelectionDAG where we try to merge 2 loads
that have the same operands but their MMO flags differ. One is
dereferenceable and one is not. I'm not sure what the underlying issue
here is:

1) MDSDNode with the same operands should have the same flags set on
their respective MMO. The fact the flags differ when the
opcode,types,operands and address-space are the same is the problem.

2) Having MDSDnodes with the same operands but different MMO flags is
possible, so the Flags should be added to the FoldingSetNodeID.

3) Something else I haven't considered.

I have a patch posted implementing 2, but don't know if I should look
at fixing 1 as well (or perhaps instead). The loads that trigger the
assertion are:

t47: v4i32,ch = load<LD16[%0+80](align=8)(dereferenceable)> t20, t46, undef:i64
t69: v4i32,ch = load<LD16[FixedStack1+80](align=8)> t50, t46, undef:i64

I would expect the the second load should also be marked
dereferenceable since its loading from one of the TargetFrames. Am I
on the right track here?

Thanks
Sean

initial_sdag.txt (6.08 KB)

MMO_Flag_Mismatch.ll (1.69 KB)

Hello,

I've hit an assertion in SelectionDAG where we try to merge 2 loads
that have the same operands but their MMO flags differ. One is
dereferenceable and one is not. I'm not sure what the underlying issue
here is:

1) MDSDNode with the same operands should have the same flags set on
their respective MMO. The fact the flags differ when the
opcode,types,operands and address-space are the same is the problem.

2) Having MDSDnodes with the same operands but different MMO flags is
possible, so the Flags should be added to the FoldingSetNodeID.

3) Something else I haven't considered.

I have a patch posted implementing 2, but don't know if I should look
at fixing 1 as well (or perhaps instead). The loads that trigger the
assertion are:

t47: v4i32,ch = load<LD16[%0+80](align=8)(dereferenceable)> t20, t46, undef:i64
t69: v4i32,ch = load<LD16[FixedStack1+80](align=8)> t50, t46, undef:i64

I would expect the the second load should also be marked
dereferenceable since its loading from one of the TargetFrames. Am I
on the right track here?

Hi, Sean,

To follow-up on this, first I’ll note that, IIRC, we’ve fixed a couple of related problems in the recent past. Specifically, the following come to mind: r309930 and r306404. In this case, it seems like something is dropping this information somewhere. Or, perhaps, I don’t see why we have a “FixedStack” location that’s not marked as dereferenceable. That having been said…

As for whether “having MDSDnodes with the same operands but different MMO flags is possible”, the answer is yes. Of the bits that are currently defined, MOLoad and MOStore are attached to fundamental properties of the corresponding SDNode. Of the remainder: MOVolatile, MONonTemporal, MODereferenceable, and MOInvariant, some of these can be set independently on a per-operation basis. For example, one can certainly have a two volatile loads of the same address, one which is non-temporal and one which is not. However, those bits are already added to the FoldingSetNodeID, because they’re mirrored by the MemSDNode’s MemSDNodeBits (which has bits for IsVolatile, IsNonTemporal, IsDereferenceable, and IsInvariant), and these should be returned by getRawSubclassData, which is called in all of the places that D38898 modifies). Thus, in short, I don’t understand why the IsDereferenceable bit is not already included in the FoldingSetNodeID.

-Hal

Hi Hal,

Thanks for the reply, that clears up a fair bit for me. I think the
reason we have the 'FixedStack' location without it being marked as
dereferenceable is because its created in CreateCopyOfByValArgument
where we use a default MachinePointerInfo in the call to getMemcpy. As
for fixing the assertion I think I understand now why we look up a
node with different MMO flags, its a 'MemIntrinsicSDNode' which
doesn't add its subclass data to the FoldingSetNode. I'll update the
patch on D38898 accordingly.

Thanks
Sean