`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 ↗

(On Diff #530049)

Ideally would have some defs for it

4198 ↗

(On Diff #530049)

can just specify the return type here

4211 ↗

(On Diff #530049)

This should really fail with use of undefined value

[niwinanto]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4197 ↗

(On Diff #530049)

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

4198 ↗

(On Diff #530049)

You meant auto MIBAtomicRMWNand to MachineInstrBuilder MIBAtomicRMWNand, or?

4211 ↗

(On Diff #530049)

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

4207 ↗

(On Diff #530187)

Also check the inputs

4197 ↗

(On Diff #530049)

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

4198 ↗

(On Diff #530049)

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

4211 ↗

(On Diff #530049)

You didn't define the input registers for the instruction

[niwinanto]

Feedback addressed for test case.

[niwinanto]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4198 ↗

(On Diff #530049)

I don't get the issue with buildAtomicRMWNand

[arsenm]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4198 ↗

(On Diff #530049)

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 ↗

(On Diff #530049)

Thanks for explaining. Now I got it.

[arsenm]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

4197 ↗

(On Diff #530228)

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: