[CodeGenSchedule] Don't allow invalid ReadAdvances to be formed

[francisvm]

Forming a ReadAdvance with an entry in the ValidWrites list that is not used by any instruction results in the entire ReadAdvance to be ignored by the scheduler due to an invalid entry.

The SchedRW collection code only picks up SchedWrites that are reachable from Instructions, InstRW, ItinRW and SchedAlias, leaving the unreachable ones with an invalid entry (0) in SubtargetEmitter::GenSchedClassTables when going through the list of ReadAdvances

[francisvm]

Mostly trying to get thoughts on this first, since current models fail the check today:

AArch64SchedExynosM4.td:497:5: error: ReadAdvance referencing a ValidWrite that is not used by any instruction (M4WriteFMAC5)

Does it make sense to make this an error? Should it be a warning? Should we make the emitter to silently skip the SchedWrite in the list but still apply the ReadAdvance to the rest of the ValidWrites?

[francisvm]

@ebahapo both the ExynosM4 and ExynosM5 models have a M5WriteFMAC5 that is not associated with any instruction. Are you missing an InstRW somewhere, or can we get rid of it from the list of ValidWrites?

def M4WriteFMAC5   : SchedWriteRes<[M4UnitFMAC]>  { let Latency = 5; }
def M4ReadFMACM1   : SchedReadAdvance<+1, [M4WriteFMAC4,
                                           M4WriteFMAC4H,
                                           M4WriteFMAC5]>;
// no other references of M4WriteFMAC5

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

Author: Visoiu Mistrih Francis (francisvm)

Changes

Forming a ReadAdvance with an entry in the ValidWrites list that is not used by any instruction results in the entire ReadAdvance to be ignored by the scheduler due to an invalid entry.

The SchedRW collection code only picks up SchedWrites that are reachable from Instructions, InstRW, ItinRW and SchedAlias, leaving the unreachable ones with an invalid entry (0) in SubtargetEmitter::GenSchedClassTables when going through the list of ReadAdvances

---

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

5 Files Affected:

• (modified) llvm/lib/Target/AArch64/AArch64SchedExynosM4.td (+1-3)
• (modified) llvm/lib/Target/AArch64/AArch64SchedExynosM5.td (+1-3)
• (added) llvm/test/TableGen/ReadAdvanceInvalidWrite.td (+29)
• (modified) llvm/utils/TableGen/CodeGenSchedule.cpp (+9)
• (modified) llvm/utils/TableGen/SubtargetEmitter.cpp (+4-1)

diff --git a/llvm/lib/Target/AArch64/AArch64SchedExynosM4.td b/llvm/lib/Target/AArch64/AArch64SchedExynosM4.td
index 5163de280f2e4f..b75264602dbc11 100644
--- a/llvm/lib/Target/AArch64/AArch64SchedExynosM4.td
+++ b/llvm/lib/Target/AArch64/AArch64SchedExynosM4.td
@@ -309,7 +309,6 @@ def M4WriteFMAC3H  : SchedWriteRes<[M4UnitFMACH]> { let Latency = 3; }
 def M4WriteFMAC3   : SchedWriteRes<[M4UnitFMAC]>  { let Latency = 3; }
 def M4WriteFMAC4   : SchedWriteRes<[M4UnitFMAC]>  { let Latency = 4; }
 def M4WriteFMAC4H  : SchedWriteRes<[M4UnitFMACH]> { let Latency = 4; }
-def M4WriteFMAC5   : SchedWriteRes<[M4UnitFMAC]>  { let Latency = 5; }
 
 def M4WriteFSQR7H  : SchedWriteRes<[M4UnitFSQRH]> { let Latency = 7;
                                                     let ReleaseAtCycles = [6]; }
@@ -495,8 +494,7 @@ def M4WriteMOVI    : SchedWriteVariant<[SchedVar<IsZeroFPIdiomPred, [M4WriteZ0]>
 // Fast forwarding.
 def M4ReadAESM1    : SchedReadAdvance<+1, [M4WriteNCRY1]>;
 def M4ReadFMACM1   : SchedReadAdvance<+1, [M4WriteFMAC4,
-                                           M4WriteFMAC4H,
-                                           M4WriteFMAC5]>;
+                                           M4WriteFMAC4H]>;
 def M4ReadNMULM1   : SchedReadAdvance<+1, [M4WriteNMUL3]>;
 def M4ReadNMULP2   : SchedReadAdvance<-2, [M4WriteNMUL3]>;
 
diff --git a/llvm/lib/Target/AArch64/AArch64SchedExynosM5.td b/llvm/lib/Target/AArch64/AArch64SchedExynosM5.td
index 2ccbe1614dcd79..6b5a6da76b3a81 100644
--- a/llvm/lib/Target/AArch64/AArch64SchedExynosM5.td
+++ b/llvm/lib/Target/AArch64/AArch64SchedExynosM5.td
@@ -338,7 +338,6 @@ def M5WriteFDIV12  : SchedWriteRes<[M5UnitFDIV]>  { let Latency = 12;
 
 def M5WriteFMAC3   : SchedWriteRes<[M5UnitFMAC]>  { let Latency = 3; }
 def M5WriteFMAC4   : SchedWriteRes<[M5UnitFMAC]>  { let Latency = 4; }
-def M5WriteFMAC5   : SchedWriteRes<[M5UnitFMAC]>  { let Latency = 5; }
 
 def M5WriteFSQR5   : SchedWriteRes<[M5UnitFSQR]>  { let Latency = 5;
                                                     let ReleaseAtCycles = [2]; }
@@ -530,8 +529,7 @@ def M5WriteMOVI    : SchedWriteVariant<[SchedVar<IsZeroFPIdiomPred, [M5WriteZ0]>
 // Fast forwarding.
 def M5ReadFM1      : SchedReadAdvance<+1, [M5WriteF2]>;
 def M5ReadAESM2    : SchedReadAdvance<+2, [M5WriteNCRY2]>;
-def M5ReadFMACM1   : SchedReadAdvance<+1, [M5WriteFMAC4,
-                                           M5WriteFMAC5]>;
+def M5ReadFMACM1   : SchedReadAdvance<+1, [M5WriteFMAC4]>;
 def M5ReadNMULM1   : SchedReadAdvance<+1, [M5WriteNMUL3]>;
 
 //===----------------------------------------------------------------------===//
diff --git a/llvm/test/TableGen/ReadAdvanceInvalidWrite.td b/llvm/test/TableGen/ReadAdvanceInvalidWrite.td
new file mode 100644
index 00000000000000..d185cbd56f8e96
--- /dev/null
+++ b/llvm/test/TableGen/ReadAdvanceInvalidWrite.td
@@ -0,0 +1,29 @@
+// RUN: not llvm-tblgen -gen-subtarget -I %p/../../include %s 2>&1 | FileCheck %s
+
+// Make sure we don't form ReadAdvances with ValidWrites entries that are not
+// associated with any instructions.
+
+include "llvm/Target/Target.td"
+
+def TargetX : Target;
+
+def WriteX : SchedWrite;
+def WriteY : SchedWrite;
+def ReadX : SchedRead;
+
+def InstX : Instruction {
+  let OutOperandList = (outs);
+  let InOperandList = (ins);
+  let SchedRW = [WriteX, ReadX];
+}
+
+def SchedModelX: SchedMachineModel {
+  let CompleteModel = 0;
+}
+
+let SchedModel = SchedModelX in {
+  def : ReadAdvance<ReadX, 1, [WriteX, WriteY]>;
+  // CHECK: error: ReadAdvance referencing a ValidWrite that is not used by any instruction (WriteY)
+}
+
+def ProcessorX: ProcessorModel<"ProcessorX", SchedModelX, []>;
diff --git a/llvm/utils/TableGen/CodeGenSchedule.cpp b/llvm/utils/TableGen/CodeGenSchedule.cpp
index b4c624703626c3..d819016f8b5610 100644
--- a/llvm/utils/TableGen/CodeGenSchedule.cpp
+++ b/llvm/utils/TableGen/CodeGenSchedule.cpp
@@ -2190,6 +2190,15 @@ void CodeGenSchedModels::addWriteRes(Record *ProcWriteResDef, unsigned PIdx) {
 // Add resources for a ReadAdvance to this processor if they don't exist.
 void CodeGenSchedModels::addReadAdvance(Record *ProcReadAdvanceDef,
                                         unsigned PIdx) {
+  for (const Record *ValidWrite :
+       ProcReadAdvanceDef->getValueAsListOfDefs("ValidWrites"))
+    if (getSchedRWIdx(ValidWrite, /*IsRead=*/false) == 0)
+      PrintFatalError(
+          ProcReadAdvanceDef->getLoc(),
+          "ReadAdvance referencing a ValidWrite that is not used by "
+          "any instruction (" +
+              ValidWrite->getName() + ")");
+
   RecVec &RADefs = ProcModels[PIdx].ReadAdvanceDefs;
   if (is_contained(RADefs, ProcReadAdvanceDef))
     return;
diff --git a/llvm/utils/TableGen/SubtargetEmitter.cpp b/llvm/utils/TableGen/SubtargetEmitter.cpp
index 2707f54eed6e9b..d350d7de139f6e 100644
--- a/llvm/utils/TableGen/SubtargetEmitter.cpp
+++ b/llvm/utils/TableGen/SubtargetEmitter.cpp
@@ -1262,7 +1262,10 @@ void SubtargetEmitter::GenSchedClassTables(const CodeGenProcModel &ProcModel,
         WriteIDs.push_back(0);
       else {
         for (Record *VW : ValidWrites) {
-          WriteIDs.push_back(SchedModels.getSchedRWIdx(VW, /*IsRead=*/false));
+          unsigned WriteID = SchedModels.getSchedRWIdx(VW, /*IsRead=*/false);
+          assert(WriteID != 0 &&
+                 "Expected a valid SchedRW in the list of ValidWrites");
+          WriteIDs.push_back(WriteID);
         }
       }
       llvm::sort(WriteIDs);

[fpetrogalli]

LGTM, thank you

[ebahapo]

@francisvm, thank you for this change.

As a matter of fact, all the proprietary Exynos processors through M5 are obsolete and so is the support for them in LLVM. However, I’m not associated with Samsung anymore.