`G_ATOMICRMW_NAND` failure in legalizer

[niwinanto]

Summary

Legalizer fails to widen output type for G_ATOMICRMW_NAND.

[arsenm]

Needs testcase

[niwinanto]

In D152538#4408800, @arsenm wrote:

Needs testcase

I am not sure how to trigger this error. I don't see any targets legalizing G_ATOMICRMW_NAND. Only AMD GPU has G_ATOMICRMW_NAND in legalizerInfo, but it is lowered. In our use case we are legalizing it and facing this issue.

[arsenm]

In D152538#4409114, @niwinanto wrote:

In D152538#4408800, @arsenm wrote:

Needs testcase

I am not sure how to trigger this error. I don't see any targets legalizing G_ATOMICRMW_NAND. Only AMD GPU has G_ATOMICRMW_NAND in legalizerInfo, but it is lowered. In our use case we are legalizing it and facing this issue.

You didn't post any patch context so I'm not sure what you're specifically handling here. You just need a mir test with G_ATOMICRMW_NAND and/or an IR test with atomicrmw nand with the appropriate type

[niwinanto]

In D152538#4409120, @arsenm wrote:

In D152538#4409114, @niwinanto wrote:

In D152538#4408800, @arsenm wrote:

Needs testcase

I am not sure how to trigger this error. I don't see any targets legalizing G_ATOMICRMW_NAND. Only AMD GPU has G_ATOMICRMW_NAND in legalizerInfo, but it is lowered. In our use case we are legalizing it and facing this issue.

You didn't post any patch context so I'm not sure what you're specifically handling here. You just need a mir test with G_ATOMICRMW_NAND and/or an IR test with atomicrmw nand with the appropriate type

Apologies, I could not explain correctly.

For example

%0 = atomicrmw nand ptr @sc, i8 1 seq_cst, align 1

to

%0:_(s8) = G_ATOMICRMW_NAND %1(p0), %2 :: (load store seq_cst (s8) on @s    c)

In our use case, s8 for G_ATOMICRMW_NAND is not supported. Hence, trying to widen it to s32.
However, LegalizerHelper::widenScalar(...) does not handle G_ATOMICRMW_NAND, but other G_ATOMICRMW*.

Why I cannot add test/ trigger this error, no other target legalize G_ATOMICRMW_NAND, hence I cannot invoke LegalizerHelper::widenScalar(...).

[arsenm]

In D152538#4409230, @niwinanto wrote:

In D152538#4409120, @arsenm wrote:

In D152538#4409114, @niwinanto wrote:

In D152538#4408800, @arsenm wrote:

Needs testcase

I am not sure how to trigger this error. I don't see any targets legalizing G_ATOMICRMW_NAND. Only AMD GPU has G_ATOMICRMW_NAND in legalizerInfo, but it is lowered. In our use case we are legalizing it and facing this issue.

You didn't post any patch context so I'm not sure what you're specifically handling here. You just need a mir test with G_ATOMICRMW_NAND and/or an IR test with atomicrmw nand with the appropriate type

Apologies, I could not explain correctly.

For example

%0 = atomicrmw nand ptr @sc, i8 1 seq_cst, align 1

to

%0:_(s8) = G_ATOMICRMW_NAND %1(p0), %2 :: (load store seq_cst (s8) on @s    c)

In our use case, s8 for G_ATOMICRMW_NAND is not supported. Hence, trying to widen it to s32.
However, LegalizerHelper::widenScalar(...) does not handle G_ATOMICRMW_NAND, but other G_ATOMICRMW*.

Why I cannot add test/ trigger this error, no other target legalize G_ATOMICRMW_NAND, hence I cannot invoke LegalizerHelper::widenScalar(...).

I think ppc or mips would use this, plus there's always unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

[niwinanto]

In D152538#4409242, @arsenm wrote:

In D152538#4409230, @niwinanto wrote:

In D152538#4409120, @arsenm wrote:

In D152538#4409114, @niwinanto wrote:

In D152538#4408800, @arsenm wrote:

Needs testcase

I am not sure how to trigger this error. I don't see any targets legalizing G_ATOMICRMW_NAND. Only AMD GPU has G_ATOMICRMW_NAND in legalizerInfo, but it is lowered. In our use case we are legalizing it and facing this issue.

You didn't post any patch context so I'm not sure what you're specifically handling here. You just need a mir test with G_ATOMICRMW_NAND and/or an IR test with atomicrmw nand with the appropriate type

Apologies, I could not explain correctly.

For example

%0 = atomicrmw nand ptr @sc, i8 1 seq_cst, align 1

to

%0:_(s8) = G_ATOMICRMW_NAND %1(p0), %2 :: (load store seq_cst (s8) on @s    c)

In our use case, s8 for G_ATOMICRMW_NAND is not supported. Hence, trying to widen it to s32.
However, LegalizerHelper::widenScalar(...) does not handle G_ATOMICRMW_NAND, but other G_ATOMICRMW*.

Why I cannot add test/ trigger this error, no other target legalize G_ATOMICRMW_NAND, hence I cannot invoke LegalizerHelper::widenScalar(...).

I think ppc or mips would use this, plus there's always unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

I was not aware of unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp let me have a look at this.

[niwinanto]

Added testcase.

[arsenm]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4197

Ideally would have some defs for it

4198

can just specify the return type here

4211

This should really fail with use of undefined value

[niwinanto]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4197

Yes, but I don't find any def for MachineMemOperand and this is how other tests are also using it.

4198

You meant auto MIBAtomicRMWNand to MachineInstrBuilder MIBAtomicRMWNand, or?

4211

I don't really get this. What do you mean?

[niwinanto]

Updated with full diff for more patch context.

[arsenm]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4197

MachineMemOperand is not a value. I'm talking about Addr and Val, which are undefined registers here

4198

auto Nand = B.buildAtomicRMW(LLT::scalar(32))

4207

Also check the inputs

4211

You didn't define the input registers for the instruction

[niwinanto]

Feedback addressed for test case.

[niwinanto]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4198

I don't get the issue with buildAtomicRMWNand

[arsenm]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4198

You do not need to pre-create the register. You can pass the type like you did with buildConstant above

[niwinanto]

More feedback addressed.

[niwinanto]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4198

Thanks for explaining. Now I got it.

[arsenm]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4197

Oh, looks like all of these wrappers around specific atomicrmws were never updated

[niwinanto]

@arsenm thank you for reviewing this. I do not have the commit access, could you please commit for me.
Niwin Anto <[e-mail]>

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

   case TargetOpcode::G_ATOMICRMW_ADD:
   case TargetOpcode::G_ATOMICRMW_SUB:
   case TargetOpcode::G_ATOMICRMW_AND:
+  case TargetOpcode::G_ATOMICRMW_NAND:
   case TargetOpcode::G_ATOMICRMW_OR:
   case TargetOpcode::G_ATOMICRMW_XOR:
   case TargetOpcode::G_ATOMICRMW_MIN:

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
+// ATOMICRMW_NAND widening.
+TEST_F(AArch64GISelMITest, WidenATOMICRMW_NAND) {
+  setUp();
+  if (!TM)
+    return;
+
+  LLT S64 = LLT::scalar(64);
+  // Declare your legalization info
+  DefineLegalizerInfo(A, {
+    getActionDefinitionsBuilder(G_ATOMICRMW_NAND)
+        .legalFor({{s64, LLT::pointer(0, 64)}});
+  });
+
+  auto OldValRes = MRI->createGenericVirtualRegister(LLT::scalar(32));
+  auto Addr = MRI->createGenericVirtualRegister(LLT::pointer(0, 64));
+  auto Val = MRI->createGenericVirtualRegister(LLT::scalar(32));
+  MachineMemOperand *MMO = MF->getMachineMemOperand(
+      MachinePointerInfo(),
+      MachineMemOperand::MOLoad | MachineMemOperand::MOStore, 8, Align(8),
+      AAMDNodes(), nullptr, SyncScope::System, AtomicOrdering::Unordered);
+
+  auto MIBAtomicRMWNand = B.buildAtomicRMWNand(OldValRes, Addr, Val, *MMO);
+
+  AInfo Info(MF->getSubtarget());
+  DummyGISelObserver Observer;
+  LegalizerHelper Helper(*MF, Info, Observer, B);
+  EXPECT_TRUE(Helper.widenScalar(*MIBAtomicRMWNand, 0, S64) ==
+              LegalizerHelper::LegalizeResult::Legalized);
+
+  auto CheckStr = R"(
+  CHECK: [[ATMNAND:%[0-9]+]]:_(s64) = G_ATOMICRMW_NAND 
+  )";
+
+  // Check
+  EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
+}
+
 } // namespace