[PATCH] D40092: [globalisel][irtranslator] Add support for atomicrmw and (strong) cmpxchg

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

LGTM nitpicks below



================
Comment at: include/llvm/Target/GenericOpcodes.td:501
+  let OutOperandList = (outs type0:$oldval);
+  let InOperandList = (ins type2:$addr, type0:$val);
+  let hasSideEffects = 0;
----------------
type2 => type1 (not super important, I believe it would work either way)


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1134
+  Type *ResType = I.getType();
+  Type *ValType = I.getType()->Type::getStructElementType(0);
+  Type *SuccessType = I.getType()->getStructElementType(1);
----------------
Use ResType instead of I.getType()


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1145
+  unsigned OldValRes = MRI->createGenericVirtualRegister(ValTy);
+  unsigned SuccessRes = MRI->createGenericVirtualRegister(SuccessTy);
+  unsigned Addr = getOrCreateVReg(*I.getPointerOperand());
----------------
Aditya pushed patches to make this kind of blob of create vregs unnecessary.
Could you use something similar here?

Edit: Oh part of this, is because we have only one value for the result, right?


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1159
+  Type *Int32Ty = Type::getInt32Ty(U.getContext());
+  SmallVector<Value *, 1> Indices;
+  // getIndexedOffsetInType is designed for GEPs, so the first index is the
----------------
SmallVect... 2?


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:710
+
+MachineInstrBuilder
+MachineIRBuilder::buildAtomicRMWXchg(unsigned OldValRes, unsigned Addr,
----------------
Could we macro-generate those?
Like
GENERATE_BUILD_ATOMICRMW(Xchg, XCHG)
GENERATE_BUILD_ATOMICRMW(Sub, SUB)

That way if we have to make a change we only have to change the macro.


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:785
   if (DstTy.isVector()) {
-    assert(SrcTy.isVector() && "mismatched cast between vecot and non-vector");
+    assert(SrcTy.isVector() && "mismatched cast between vector and non-vector");
     assert(SrcTy.getNumElements() == DstTy.getNumElements() &&
----------------
Different NFC patch ;).


https://reviews.llvm.org/D40092