[PATCH] D152538: `G_ATOMICRMW_NAND` failure in legalizer

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp:4207
+  auto CheckStr = R"(
+  CHECK: [[ATMNAND:%[0-9]+]]:_(s64) = G_ATOMICRMW_NAND 
+  )";
----------------
Also check the inputs


================
Comment at: llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp:4197
+      AAMDNodes(), nullptr, SyncScope::System, AtomicOrdering::Unordered);
+
+  auto MIBAtomicRMWNand = B.buildAtomicRMWNand(OldValRes, Addr, Val, *MMO);
----------------
niwinanto wrote:
> arsenm wrote:
> > Ideally would have some defs for it
> Yes, but I don't find any def for `MachineMemOperand` and this is how other tests are also using it.
MachineMemOperand is not a value. I'm talking about Addr and Val, which are undefined registers here


================
Comment at: llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp:4198
+
+  auto MIBAtomicRMWNand = B.buildAtomicRMWNand(OldValRes, Addr, Val, *MMO);
+
----------------
niwinanto wrote:
> arsenm wrote:
> > can just specify the return type here
> You meant `auto MIBAtomicRMWNand` to `MachineInstrBuilder MIBAtomicRMWNand`, or?
auto Nand = B.buildAtomicRMW(LLT::scalar(32))


================
Comment at: llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp:4211
+  // Check
+  EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
+}
----------------
niwinanto wrote:
> arsenm wrote:
> > This should really fail with use of undefined value
> I don't really get this. What do you mean?
You didn't define the input registers for the instruction


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152538/new/

https://reviews.llvm.org/D152538