[SelectionDAG] Add MachineMemOperand flags to the NodeID for MemSDNodes

[sfertile]

Summary

During lowering we try to combine 2 loads with the same opcodes and operands but different flags on the MMO which triggers the assertion 'llvm::MachineMemOperand::refineAlignment(const llvm::MachineMemOperand*): Assertion MMO->getFlags() == getFlags() && "Flags mismatch!"' failed.

The initial selection dag ends up with 2 LD16s of 'FrameIndex 1 + 80' but one MMO has the dereferenceable flag and one does not.

t20: ch = lifetime.start t17, TargetFrameIndex:i64<0>
t46: i64 = add FrameIndex:i64<1>, Constant:i64<80>
t47: v4i32,ch = load<LD16[%0+80](align=8)(dereferenceable)> t20, t46, undef:i64
t50: ch = TokenFactor t24:1, t25, t28:1, t30, t32:1, t34, t37:1, t39, t42:1, t44, t47:1, t49
t69: v4i32,ch = load<LD16[FixedStack1+80](align=8)> t50, t46, undef:i64

While trying to lower t47 to a target specific LXVD2X we end up looking up the already lowered node for t69 which now has the same operands and we assert when trying to refine the alignment between the 2 MMOs.

I've added the subclass data to the FoldingSetNode for MemIntrinsicSDNodes which will make the relevant flags part of its signature.

[hfinkel]

These flags should essentially already be added by the calls to ID.AddInteger(getSyntheticNodeSubclassData(... and ID.AddInteger(ST->getRawSubclassData()) and friends. Is this not happening uniformly?

[sfertile]

Changed direction due to my new understanding. I've added the subclass data to the FoldingSetNode for MemIntrinsicSDNodes.

[hfinkel]

LGTM

Also, do we have a problem with prefetches in this regard too. For ISD::PREFETCH I see just this:

case ISD::PREFETCH: {
  const MemSDNode *PF = cast<MemSDNode>(N);
  ID.AddInteger(PF->getPointerInfo().getAddrSpace());
  break;
}

If you agree that there's also a problem here, please update this as well.

[sfertile]

In D38898#908836, @hfinkel wrote:

LGTM

Also, do we have a problem with prefetches in this regard too. For ISD::PREFETCH I see just this:

case ISD::PREFETCH: {
  const MemSDNode *PF = cast<MemSDNode>(N);
  ID.AddInteger(PF->getPointerInfo().getAddrSpace());
  break;
}

If you agree that there's also a problem here, please update this as well.

I agree, I'll update and see if I can produce a test case for it.

include/llvm/CodeGen/SelectionDAG.h

static uint16_t getSyntheticNodeSubclassData(unsigned IROrder,
                                                ArgTypes &&... Args) {
     // The compiler can reduce this expression to a constant iff we pass an
     // empty DebugLoc.  Thankfully, the debug location doesn't have any bearing
     // on the subclass data.
     return SDNodeT(IROrder, DebugLoc(), std::forward<ArgTypes>(Args)...)
         .getRawSubclassData();
   }
 
+  template <typename SDNodeTy>
+  static uint16_t getSyntheticNodeSubclassData(unsigned Opc, unsigned Order,
+                                                SDVTList VTs, EVT MemoryVT,
+                                                MachineMemOperand *MMO) {
+    return SDNodeTy(Opc, Order, DebugLoc(), VTs, MemoryVT, MMO)
+         .getRawSubclassData();
+  }
+
   void createOperands(SDNode *Node, ArrayRef<SDValue> Vals) {
     assert(!Node->OperandList && "Node already has operands");
     SDUse *Ops = OperandRecycler.allocate(
         ArrayRecycler<SDUse>::Capacity::get(Vals.size()), OperandAllocator);
 
     for (unsigned I = 0; I != Vals.size(); ++I) {
       Ops[I].setUser(Node);
       Ops[I].setInitial(Vals[I]);

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

assert((Opcode == ISD::INTRINSIC_VOID ||
            (int)Opcode >= ISD::FIRST_TARGET_MEMORY_OPCODE)) &&
          "Opcode is not a memory-accessing opcode!");
 
   // Memoize the node unless it returns a flag.
   MemIntrinsicSDNode *N;
   if (VTList.VTs[VTList.NumVTs-1] != MVT::Glue) {
     FoldingSetNodeID ID;
     AddNodeIDNode(ID, Opcode, VTList, Ops);
+    ID.AddInteger(getSyntheticNodeSubclassData<MemIntrinsicSDNode>(
+        Opcode, dl.getIROrder(), VTList, MemVT, MMO));
     ID.AddInteger(MMO->getPointerInfo().getAddrSpace());
     void *IP = nullptr;
     if (SDNode *E = FindNodeOrInsertPos(ID, dl, IP)) {
       cast<MemIntrinsicSDNode>(E)->refineAlignment(MMO);
       return SDValue(E, 0);
     }
 
     N = newSDNode<MemIntrinsicSDNode>(Opcode, dl.getIROrder(), dl.getDebugLoc(),

test/CodeGen/PowerPC/MMO-flags-assertion.ll

+; RUN: llc < %s -mtriple powerpc64le-unknown-linux-gnu
+
+; void llvm::MachineMemOperand::refineAlignment(const llvm::MachineMemOperand*):
+; Assertion `MMO->getFlags() == getFlags() && "Flags mismatch !"' failed.
+
+declare void @_Z3fn11F(%class.F* byval align 8) local_unnamed_addr
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i32, i1)
+declare signext i32 @_ZN1F11isGlobalRegEv(%class.F*) local_unnamed_addr
+declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
+declare void @_Z10EmitLValuev(%class.F* sret) local_unnamed_addr
+declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
+
+%class.F = type { i32, i64, i8, [64 x i8], i8, i32* }
+
+define signext i32 @_Z29EmitOMPAtomicSimpleUpdateExpr1F(%class.F* byval align 8 %p1) local_unnamed_addr {
+entry:
+  call void @_Z3fn11F(%class.F* byval nonnull align 8 %p1)
+  %call = call signext i32 @_ZN1F11isGlobalRegEv(%class.F* nonnull %p1)
+  ret i32 %call
+}
+
+define void @_Z3fn2v() local_unnamed_addr {
+entry:
+  %agg.tmp1 = alloca %class.F, align 8
+  %XLValue = alloca %class.F, align 8
+  %0 = bitcast %class.F* %XLValue to i8*
+  call void @llvm.lifetime.start.p0i8(i64 96, i8* nonnull %0)
+  call void @_Z10EmitLValuev(%class.F* nonnull sret %XLValue)
+  %1 = bitcast %class.F* %agg.tmp1 to i8*
+  call void @llvm.lifetime.start.p0i8(i64 96, i8* nonnull %1)
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull %1, i8* nonnull %0, i64 96, i32 8, i1 false)
+  call void @_Z3fn11F(%class.F* byval nonnull align 8 %XLValue)
+  %call.i = call signext i32 @_ZN1F11isGlobalRegEv(%class.F* nonnull %agg.tmp1)
+  call void @llvm.lifetime.end.p0i8(i64 96, i8* nonnull %1)
+  call void @llvm.lifetime.end.p0i8(i64 96, i8* nonnull %0)
+  ret void
+}