[AArch64] Put legal action first for G_ATOMIC_CMPXCHG

[RoboTux]

No description provided.

[llvmbot]

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-aarch64

Author: Thomas Preud'homme (RoboTux)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/74613.diff

1 Files Affected:

• (modified) llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp (+3-3)

diff --git a/llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp b/llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
index 21a412e9360dc..7df49d188c332 100644
--- a/llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
+++ b/llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
@@ -758,11 +758,11 @@ AArch64LegalizerInfo::AArch64LegalizerInfo(const AArch64Subtarget &ST)
           all(typeInSet(0, {s8, s16, s32, s64, s128}), typeIs(2, p0)));
 
   getActionDefinitionsBuilder(G_ATOMIC_CMPXCHG)
+      .clampScalar(0, s32, s128)
+      .legalIf(all(typeInSet(0, {s32, s64}), typeIs(1, p0)))
       .customIf([](const LegalityQuery &Query) {
         return Query.Types[0].getSizeInBits() == 128;
-      })
-      .clampScalar(0, s32, s64)
-      .legalIf(all(typeInSet(0, {s32, s64}), typeIs(1, p0)));
+      });
 
   getActionDefinitionsBuilder(
       {G_ATOMICRMW_XCHG, G_ATOMICRMW_ADD, G_ATOMICRMW_SUB, G_ATOMICRMW_AND,

[RoboTux]

That should become a simple custom()

[RoboTux]

As pointed by Pavel, typeIs(0, s128) is not the same as testing that the size in bits is 128 due to vector. Although the intent is probably to former, keeping the existing check makes this more of an NFC.

[ilinpv]

Looks like rule on 128-bit vector types case was removed, but not sure if it was needed. Is the patch meant to be NFC?

[davemgreen]

(It might be better to keep it in just to be explicit about the types that are supported.)

[RoboTux]

I don't follow, the customIf checks for typeIs(0, s128). Is that different from the original check?

[davemgreen]

Sorry I think I was looking at an older version. I think you could probably use customFor({s128, p0}), but either way this looks OK to me.

[RoboTux]

@ilinpv pointed out that testing for s128 is not equivalent to what was done so I've changed to keep the old test to keep this more of an RFC. Strangely, it required updating the legalizer-info-validation test. Would you happen to know why that changed?

[ilinpv]

I think rules might be simplified in the next patch, this one looks good to me.