[TableGen][SystemZ] Correctly check the range of a leaf immediate

[s-barannikov]

The "Size >= 32" check probably dates back to when TableGen integers were 32-bit. Delete it and simplify code by using isInt/isUInt.

[llvmbot]

@llvm/pr-subscribers-tablegen

@llvm/pr-subscribers-backend-systemz

Author: Sergei Barannikov (s-barannikov)

Changes

The "Size >= 32" check probably dates back to when TableGen integers were 32-bit. Delete it and simplify code by using isInt/isUInt.

---

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

3 Files Affected:

• (modified) llvm/lib/Target/SystemZ/SystemZInstrInfo.td (+4-4)
• (modified) llvm/lib/Target/SystemZ/SystemZOperators.td (+11-8)
• (modified) llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp (+8-12)

diff --git a/llvm/lib/Target/SystemZ/SystemZInstrInfo.td b/llvm/lib/Target/SystemZ/SystemZInstrInfo.td
index d6cddeb8b6c303..e70ae5dadcb02d 100644
--- a/llvm/lib/Target/SystemZ/SystemZInstrInfo.td
+++ b/llvm/lib/Target/SystemZ/SystemZInstrInfo.td
@@ -951,10 +951,10 @@ def IILL : BinaryRI<"iill", 0xA53, insertll, GR32, imm32ll16>;
 def IILH : BinaryRI<"iilh", 0xA52, insertlh, GR32, imm32lh16>;
 def IIHL : BinaryRI<"iihl", 0xA51, insertll, GRH32, imm32ll16>;
 def IIHH : BinaryRI<"iihh", 0xA50, insertlh, GRH32, imm32lh16>;
-def IILL64 : BinaryAliasRI<insertll, GR64, imm64ll16>;
-def IILH64 : BinaryAliasRI<insertlh, GR64, imm64lh16>;
-def IIHL64 : BinaryAliasRI<inserthl, GR64, imm64hl16>;
-def IIHH64 : BinaryAliasRI<inserthh, GR64, imm64hh16>;
+def IILL64 : BinaryAliasRI<insertll64, GR64, imm64ll16>;
+def IILH64 : BinaryAliasRI<insertlh64, GR64, imm64lh16>;
+def IIHL64 : BinaryAliasRI<inserthl64, GR64, imm64hl16>;
+def IIHH64 : BinaryAliasRI<inserthh64, GR64, imm64hh16>;
 
 // ...likewise for 32-bit immediates.  For GR32s this is a general
 // full-width move.  (We use IILF rather than something like LLILF
diff --git a/llvm/lib/Target/SystemZ/SystemZOperators.td b/llvm/lib/Target/SystemZ/SystemZOperators.td
index 90fb4e5f370dab..6439c82d26ff5c 100644
--- a/llvm/lib/Target/SystemZ/SystemZOperators.td
+++ b/llvm/lib/Target/SystemZ/SystemZOperators.td
@@ -759,14 +759,17 @@ defm block_xor : block_op<xor>;
 // Insertions.
 def inserti8 : PatFrag<(ops node:$src1, node:$src2),
                        (or (and node:$src1, -256), node:$src2)>;
-def insertll : PatFrag<(ops node:$src1, node:$src2),
-                       (or (and node:$src1, 0xffffffffffff0000), node:$src2)>;
-def insertlh : PatFrag<(ops node:$src1, node:$src2),
-                       (or (and node:$src1, 0xffffffff0000ffff), node:$src2)>;
-def inserthl : PatFrag<(ops node:$src1, node:$src2),
-                       (or (and node:$src1, 0xffff0000ffffffff), node:$src2)>;
-def inserthh : PatFrag<(ops node:$src1, node:$src2),
-                       (or (and node:$src1, 0x0000ffffffffffff), node:$src2)>;
+
+class inserti16<int mask> : PatFrag<(ops node:$src1, node:$src2),
+                                    (or (and node:$src1, mask), node:$src2)>;
+
+def insertll   : inserti16<0xffff0000>;
+def insertlh   : inserti16<0x0000ffff>;
+def insertll64 : inserti16<0xffffffffffff0000>;
+def insertlh64 : inserti16<0xffffffff0000ffff>;
+def inserthl64 : inserti16<0xffff0000ffffffff>;
+def inserthh64 : inserti16<0x0000ffffffffffff>;
+
 def insertlf : PatFrag<(ops node:$src1, node:$src2),
                        (or (and node:$src1, 0xffffffff00000000), node:$src2)>;
 def inserthf : PatFrag<(ops node:$src1, node:$src2),
diff --git a/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp b/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
index 10f6590e9c7aa3..31bf9a98943e56 100644
--- a/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
@@ -2463,20 +2463,16 @@ bool TreePatternNode::ApplyTypeConstraints(TreePattern &TP, bool NotRegisters) {
         // Can only check for types of a known size
         if (VT == MVT::iPTR)
           continue;
-        unsigned Size = MVT(VT).getFixedSizeInBits();
-        // Make sure that the value is representable for this type.
-        if (Size >= 32)
-          continue;
+
         // Check that the value doesn't use more bits than we have. It must
         // either be a sign- or zero-extended equivalent of the original.
-        int64_t SignBitAndAbove = II->getValue() >> (Size - 1);
-        if (SignBitAndAbove == -1 || SignBitAndAbove == 0 ||
-            SignBitAndAbove == 1)
-          continue;
-
-        TP.error("Integer value '" + Twine(II->getValue()) +
-                 "' is out of range for type '" + getEnumName(VT) + "'!");
-        break;
+        unsigned Width = MVT(VT).getFixedSizeInBits();
+        int64_t Val = II->getValue();
+        if (!isIntN(Width, Val) && !isUIntN(Width, Val)) {
+          TP.error("Integer value '" + Twine(Val) +
+                   "' is out of range for type '" + getEnumName(VT) + "'!");
+          break;
+        }
       }
       return MadeChange;
     }

[s-barannikov]

This one doesn't fit in i32 and triggered the error. Other changes are for consistency.

[topperc]

LGTM

[topperc]

Should probably put [SystemZ] in the title

[uweigand]

It seems to me this pattern isn't really restricted to 16 bits, but could be used to implement all the insertion fragments (8-, 16- and 32-bit). Might be an additional simplification. Either way, LGTM. Thanks!

[s-barannikov]

Done in #119962