[AMDGPU][NFC] Extend PredicateControl to support True16 predicates.

[kosarev]

Using OtherPredicates for True16 predicates is often problematic due to interference with other kinds of predicates, particularly when this overrides predicates inherited from pseudo instructions.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Ivan Kosarev (kosarev)

Changes

Using OtherPredicates for True16 predicates is often problematic due to interference with other kinds of predicates, particularly when this overrides predicates inherited from pseudo instructions.

---

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

5 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/AMDGPU.td (+4-3)
• (modified) llvm/lib/Target/AMDGPU/AMDGPUInstructions.td (-22)
• (added) llvm/lib/Target/AMDGPU/AMDGPUPredicateControl.td (+36)
• (modified) llvm/lib/Target/AMDGPU/R600.td (+1)
• (modified) llvm/lib/Target/AMDGPU/VOPInstructions.td (+2-2)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPU.td b/llvm/lib/Target/AMDGPU/AMDGPU.td
index b29dc1edbaaff9..7c278fd574ede5 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPU.td
+++ b/llvm/lib/Target/AMDGPU/AMDGPU.td
@@ -9,6 +9,7 @@
 include "llvm/TableGen/SearchableTable.td"
 include "llvm/Target/Target.td"
 include "AMDGPUFeatures.td"
+include "AMDGPUPredicateControl.td"
 
 def p0 : PtrValueType<i64, 0>;
 def p1 : PtrValueType<i64, 1>;
@@ -1895,10 +1896,10 @@ def NotHasTrue16BitInsts : Predicate<"!Subtarget->hasTrue16BitInsts()">;
 // True16 instructions as they are defined in the ISA. Fake True16
 // instructions have the same encoding as real ones but syntactically
 // only allow 32-bit registers in operands and use low halves thereof.
-def UseRealTrue16Insts : Predicate<"Subtarget->useRealTrue16Insts()">,
+def UseRealTrue16Insts : True16PredicateClass<"Subtarget->useRealTrue16Insts()">,
   AssemblerPredicate<(all_of FeatureTrue16BitInsts, FeatureRealTrue16Insts)>;
-def UseFakeTrue16Insts : Predicate<"Subtarget->hasTrue16BitInsts() && "
-                                   "!Subtarget->useRealTrue16Insts()">;
+def UseFakeTrue16Insts : True16PredicateClass<"Subtarget->hasTrue16BitInsts() && "
+                                              "!Subtarget->useRealTrue16Insts()">;
 
 def HasVOP3PInsts : Predicate<"Subtarget->hasVOP3PInsts()">,
   AssemblerPredicate<(all_of FeatureVOP3P)>;
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td b/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
index 360aafedc52248..5ed82c0c4b1b8e 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
+++ b/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
@@ -76,33 +76,11 @@ class ILFormat<dag outs, dag ins, string asmstr, list<dag> pattern>
      let isCodeGenOnly = 1;
 }
 
-def TruePredicate : Predicate<"">;
-
-// FIXME: Tablegen should specially supports this
-def FalsePredicate : Predicate<"false">;
-
-// Add a predicate to the list if does not already exist to deduplicate it.
-class PredConcat<list<Predicate> lst, Predicate pred> {
-  list<Predicate> ret = !listconcat(lst, !listremove([pred], lst));
-}
-
 // Get the union of two Register lists
 class RegListUnion<list<Register> lstA, list<Register> lstB> {
   list<Register> ret = !listconcat(lstA, !listremove(lstB, lstA));
 }
 
-class PredicateControl {
-  Predicate SubtargetPredicate = TruePredicate;
-  Predicate AssemblerPredicate = TruePredicate;
-  Predicate WaveSizePredicate = TruePredicate;
-  list<Predicate> OtherPredicates = [];
-  list<Predicate> Predicates = PredConcat<
-                                 PredConcat<PredConcat<OtherPredicates,
-                                                       SubtargetPredicate>.ret,
-                                            AssemblerPredicate>.ret,
-                                 WaveSizePredicate>.ret;
-}
-
 class AMDGPUPat<dag pattern, dag result> : Pat<pattern, result>,
       PredicateControl, GISelFlags;
 
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUPredicateControl.td b/llvm/lib/Target/AMDGPU/AMDGPUPredicateControl.td
new file mode 100644
index 00000000000000..6c7c91ef464dfd
--- /dev/null
+++ b/llvm/lib/Target/AMDGPU/AMDGPUPredicateControl.td
@@ -0,0 +1,36 @@
+//===-- AMDGPUPredicateControl.td --------------------------*- tablegen -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+def TruePredicate : Predicate<"">;
+
+// FIXME: Tablegen should specially supports this
+def FalsePredicate : Predicate<"false">;
+
+// Add a predicate to the list if does not already exist to deduplicate it.
+class PredConcat<Predicate pred, list<Predicate> lst> {
+  list<Predicate> ret = !listconcat(lst, !listremove([pred], lst));
+}
+
+// Prevent using other kinds of predicates where True16 predicates are
+// expected by giving them their own class.
+class True16PredicateClass<string cond> : Predicate<cond>;
+def NoTrue16Predicate : True16PredicateClass<"">;
+
+class PredicateControl {
+  Predicate SubtargetPredicate = TruePredicate;
+  Predicate AssemblerPredicate = TruePredicate;
+  Predicate WaveSizePredicate = TruePredicate;
+  True16PredicateClass True16Predicate = NoTrue16Predicate;
+  list<Predicate> OtherPredicates = [];
+  list<Predicate> Predicates =
+      PredConcat<SubtargetPredicate,
+      PredConcat<AssemblerPredicate,
+      PredConcat<WaveSizePredicate,
+      PredConcat<True16Predicate,
+      OtherPredicates>.ret>.ret>.ret>.ret;
+}
diff --git a/llvm/lib/Target/AMDGPU/R600.td b/llvm/lib/Target/AMDGPU/R600.td
index 1e8e6f34fbc0cd..9148edb92b084d 100644
--- a/llvm/lib/Target/AMDGPU/R600.td
+++ b/llvm/lib/Target/AMDGPU/R600.td
@@ -35,6 +35,7 @@ include "R600Schedule.td"
 include "R600Processors.td"
 include "R600InstrInfo.td"
 include "AMDGPUInstrInfo.td"
+include "AMDGPUPredicateControl.td"
 include "AMDGPUInstructions.td"
 include "R600Instructions.td"
 include "R700Instructions.td"
diff --git a/llvm/lib/Target/AMDGPU/VOPInstructions.td b/llvm/lib/Target/AMDGPU/VOPInstructions.td
index dc6a48b1df7472..29c30bc019ed63 100644
--- a/llvm/lib/Target/AMDGPU/VOPInstructions.td
+++ b/llvm/lib/Target/AMDGPU/VOPInstructions.td
@@ -1484,8 +1484,8 @@ multiclass VOP3_Real_dpp8_with_name<GFXGen Gen, bits<10> op, string opName,
   let AsmString = asmName # ps.Pfl.AsmVOP3DPP8,
       DecoderNamespace = "DPP8"#Gen.DecoderNamespace#
                          !if(ps.Pfl.IsRealTrue16, "", "_FAKE16"),
-      OtherPredicates = !if(ps.Pfl.IsRealTrue16, [UseRealTrue16Insts],
-                            [TruePredicate]) in {
+      True16Predicate = !if(ps.Pfl.IsRealTrue16, UseRealTrue16Insts,
+                            NoTrue16Predicate) in {
     defm NAME : VOP3_Real_dpp8_Base<Gen, op, opName>;
   }
 }

[jayfoad]

LGTM but please wait a day for any other comments.

[rampitec]

LGTM

[changpeng]

There are other places in the same file that also need to be taken care of:
VOP3_DPP16_Gen, VOP3_Real_Gen for example.
Do we actually have to introduce True16Predicate?
I think we have already used listconcat to add a new predicate into OtherPredicates list.

[kosarev]

Pasting OtherPredicates could help your particular case, but wouldn't resolve the more general problem of overriding predicates for various groups of instructions and inheriting them from their corresponding pseudos.

The idea is to start using True16Predicate gradually as will seem necessary for future patches.

[changpeng]

Do you have any examples that we can not use pasting to resolve the overriding issue for OtherPredicates?

[kosarev]

In real instructions we can paste with OtherPredicates of their pseudos, but there's no way to extend self OtherPredicates of classes and records. So every let OtherPredicates = ... potentially overrides some needed predicates and so having True16Predicate makes it a bit easier to avoid clashes.

[changpeng]

It is Okay to me to introduce True16Predicate if we don't know what is exactly in OtherPredicates when we try to extend it.