[AArch64] Combine separate vector and scalar tablegen SDNode record for AArch64ISD::REV16. NFC

[topperc]

Relax the SDTypeProfile for AArch64ISD::REV32/REV64 to remove the requirement that the type be vector.

It's not a good idea to have two different SDNode records with different SDTypeProfiles. SDTypeProfiles are used to remove some unneeded checks from the GenDAGISel.inc. Having different SDTypeProfiles can cause checks to be removed that can create ambiguous matches, but that did not happen in this case.

With this change the AArchGenDAGISel.inc is identical. The only change is AArch64GenGlobalISel.inc which now includes scalar patterns for G_REV16 due to them now being picks up by an SDNodeEquiv. GISel does not yet use G_REV16 for scalars so this is not a functional change.

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

Author: Craig Topper (topperc)

Changes

It's not a good idea to have two different SDNode records with different SDTypeProfiles. SDTypeProfiles are used to remove some unneeded checks from the GenDAGISel.inc. Having different SDTypeProfiles can cause checks to be removed that can create ambiguous matches, but that did not happen in this case.

With this change the AArchGenDAGISel.inc is identical. The only change is AArch64GenGlobalISel.inc which now includes scalar patterns for G_REV16 due to them now being picks up by an SDNodeEquiv. GISel does not yet use G_REV16 for scalars so this is not a functional change.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.td (+3-5)

diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.td b/llvm/lib/Target/AArch64/AArch64InstrInfo.td
index 3c57ba414b2bf0..88a061e8f577db 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.td
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.td
@@ -817,9 +817,7 @@ def AArch64mvni_msl : SDNode<"AArch64ISD::MVNImsl", SDT_AArch64MOVIshift>;
 def AArch64movi : SDNode<"AArch64ISD::MOVI", SDT_AArch64MOVIedit>;
 def AArch64fmov : SDNode<"AArch64ISD::FMOV", SDT_AArch64MOVIedit>;
 
-def AArch64rev16_scalar : SDNode<"AArch64ISD::REV16", SDTIntUnaryOp>;
-
-def AArch64rev16 : SDNode<"AArch64ISD::REV16", SDT_AArch64UnaryVec>;
+def AArch64rev16 : SDNode<"AArch64ISD::REV16", SDTIntUnaryOp>;
 def AArch64rev32 : SDNode<"AArch64ISD::REV32", SDT_AArch64UnaryVec>;
 def AArch64rev64 : SDNode<"AArch64ISD::REV64", SDT_AArch64UnaryVec>;
 def AArch64ext : SDNode<"AArch64ISD::EXT", SDT_AArch64ExtVec>;
@@ -3000,8 +2998,8 @@ def : Pat<(bswap (rotr GPR64:$Rn, (i64 32))), (REV32Xr GPR64:$Rn)>;
 def : Pat<(srl (bswap top16Zero:$Rn), (i64 16)), (REV16Wr GPR32:$Rn)>;
 def : Pat<(srl (bswap top32Zero:$Rn), (i64 32)), (REV32Xr GPR64:$Rn)>;
 
-def : Pat<(AArch64rev16_scalar GPR32:$Rn), (REV16Wr GPR32:$Rn)>;
-def : Pat<(AArch64rev16_scalar GPR64:$Rn), (REV16Xr GPR64:$Rn)>;
+def : Pat<(AArch64rev16 GPR32:$Rn), (REV16Wr GPR32:$Rn)>;
+def : Pat<(AArch64rev16 GPR64:$Rn), (REV16Xr GPR64:$Rn)>;
 
 def : Pat<(or (and (srl GPR64:$Rn, (i64 8)), (i64 0x00ff00ff00ff00ff)),
               (and (shl GPR64:$Rn, (i64 8)), (i64 0xff00ff00ff00ff00))),

[s-barannikov]

Note that this makes the vector case less constrained (and inconsistent with REV32/REV64, which are vector-only). I think it would be better to introduce a new enum value for the scalar case.
Is is only used in one place.

[topperc]

Note that this makes the vector case less constrained (and inconsistent with REV32/REV64, which are vector-only). I think it would be better to introduce a new enum value for the scalar case. Is is only used in one place.

Does that constraint on vector provide any value? Conceptually its the same operation, rev16 on elements if you consider scalar to be a single element. It's like ISD::BSWAP but operating on 16 bits instead of bytes. I can change all of them to use SDTIntUnaryOp.

[topperc]

Note that this makes the vector case less constrained (and inconsistent with REV32/REV64, which are vector-only). I think it would be better to introduce a new enum value for the scalar case. Is is only used in one place.

Does that constraint on vector provide any value? Conceptually its the same operation, rev16 on elements if you consider scalar to be a single element. It's like ISD::BSWAP but operating on 16 bits instead of bytes. I can change all of them to use SDTIntUnaryOp.

Unfortunately, AArch64ISD::REV32/64 are used with FP vector types. So SDTIntUnaryOp doesn't work. We would need an SDT with just SDTCisSameAs<0,1>.

[s-barannikov]

Does that constraint on vector provide any value?

It would make SelectionDAG reject mixed-type cases (scalar+vector), I guess.
The current SDTCisSameAs looks even better and I also agree that conceptually it is the same operation, so LGTM.

[davemgreen]

I believe we should be safe given we only generate nodes with the types that are valid, and the classes are different. We can always add a new node if we need.