[TableGen][SchedModels] Fix read/write variant substitution

[evgeny777]

Summary

ATM, this doesn't work:

// Model1.td
let SchedModel = Model1 in {
   def Model1ReadMAC64Lo : SchedReadVariant<...>;
   def : SchedAlias<ReadMac64Lo, Model1ReadMAC64Lo>;
}

// Model2.td
let SchedModel = Model2 in {
   def Model2WriteMAC64Lo : SchedWriteVariant<...>;
   def : SchedAlias<WriteMac64Lo, Model2WriteMAC64Lo>;
}

Trying to compile this will result in TableGen error, because TableGen fails to process case when:
a) Sched class has both read and write variants
b) Read and write variants reside in different processor models

I've changed WriteMAC64Lo in cortex-a57 model in order to test this. According to optimization guide multiply long operations, which set flags, should take 1 cycle longer and have additional integer uop.

[dmgreen]

Can you upload with context?

llvm/test/tools/llvm-mca/ARM/cortex-a57-basic-instructions.s

2287

These now use L somehow? Am I reading that right?

2287

Oh. Is A57Write_4cyc_1M incorrectly specified?

[evgeny777]

llvm/test/tools/llvm-mca/ARM/cortex-a57-basic-instructions.s

2287

def A57Write_4cyc_1M  : SchedWriteRes<[A57UnitL]> { let Latency = 4;  }

Likely.

[evgeny777]

Ping

[dmgreen]

Can you rebase and upload with context?

[evgeny777]

Addressed

[dmgreen]

Thanks, A57 changes sounds OK to me.

I see code in build/lib/Target/AArch64/AArch64GenSubtargetInfo.inc that has changed like this:

  case 951: // STRDroX                                   |  25643   case 951: // STRDroX                                      
    if (SchedModel->getProcessorID() == 1) { // CycloneMo|  25644     if (SchedModel->getProcessorID() == 1) { // CycloneModel
      if (AArch64InstrInfo::isScaledAddr(*MI))           |  25645       if (AArch64InstrInfo::isScaledAddr(*MI))              
        return 1053; // (WriteIS_WriteST)_ReadBaseRS     |  25646         return 1055; // (WriteIS_WriteST)                   
      return 1054; // WriteST_ReadDefault                |  25647       return 1056; // WriteST                               
---------------------------------------------------------|  25648       if (AArch64InstrInfo::isScaledAddr(*MI))              
---------------------------------------------------------|  25649         return 1053; // _ReadBaseRS                         
---------------------------------------------------------|  25650       return 1054; // _ReadDefault                          
    }                                                    |  25651     }

Is that the merging reads and writes if they share the same processor model?

llvm/utils/TableGen/CodeGenSchedule.cpp

1672

I feel like we might need to do this.

[evgeny777]

@dmgreen Nice find! I've fixed it

[dmgreen]

LGTM. Thanks for the update.

llvm/utils/TableGen/CodeGenSchedule.cpp

1592

Perhaps now use a range based for, and no brackets.

llvm/lib/Target/ARM/ARMScheduleA57.td

 def : ReadAdvance<ReadMUL, 0>;
 
 // Multiply accumulate: MLA, MLS, SMLABB, SMLABT, SMLATB, SMLATT, SMLAWB,
 // SMLAWT, SMLAD{X}, SMLSD{X}, SMMLA{R}, SMMLS{R}
 // Multiply-accumulate pipelines support late-forwarding of accumulate operands
 // from similar μops, allowing a typical sequence of multiply-accumulate μops
 // to issue one every 1 cycle (sched advance = 2).
 def A57WriteMLA : SchedWriteRes<[A57UnitM]> { let Latency = 3; }
-def A57WriteMLAL : SchedWriteRes<[A57UnitM]> { let Latency = 4; }
+def A57WriteMLAL : SchedWriteVariant<[
+  SchedVar<IsCPSRDefinedPred, [A57Write_5cyc_1I_1M]>,
+  SchedVar<NoSchedPred,       [A57Write_4cyc_1M]>
+]>;
+
 def A57ReadMLA  : SchedReadAdvance<2, [A57WriteMLA, A57WriteMLAL]>;
 
 def : InstRW<[A57WriteMLA],
   (instregex "t2SMLAD", "t2SMLADX", "t2SMLSD", "t2SMLSDX")>;
 
 def : SchedAlias<WriteMAC16, A57WriteMLA>;
 def : SchedAlias<WriteMAC32, A57WriteMLA>;
 def : SchedAlias<ReadMAC,    A57ReadMLA>;

llvm/test/tools/llvm-mca/ARM/cortex-a57-basic-instructions.s

 # CHECK-NEXT:  1      3     1.00                        smlabtle  r5, r6, r4, r1
 # CHECK-NEXT:  1      3     1.00                        smlatbne  r4, r2, r3, r2
 # CHECK-NEXT:  1      3     1.00                        smlatteq  r8, r3, r8, r4
 # CHECK-NEXT:  1      3     1.00                        smlad r2, r3, r5, r8
 # CHECK-NEXT:  1      3     1.00                        smladx  r2, r3, r5, r8
 # CHECK-NEXT:  1      3     1.00                        smladeq r2, r3, r5, r8
 # CHECK-NEXT:  1      3     1.00                        smladxhi  r2, r3, r5, r8
 # CHECK-NEXT:  2      4     2.00                        smlal r2, r3, r5, r8
-# CHECK-NEXT:  2      4     2.00                        smlals  r2, r3, r5, r8
+# CHECK-NEXT:  4      5     2.00                        smlals  r2, r3, r5, r8
 # CHECK-NEXT:  2      4     2.00                        smlaleq r2, r3, r5, r8
-# CHECK-NEXT:  2      4     2.00                        smlalshi  r2, r3, r5, r8
+# CHECK-NEXT:  4      5     2.00                        smlalshi  r2, r3, r5, r8
 # CHECK-NEXT:  2      4     2.00                        smlalbb r3, r1, r9, r0
 # CHECK-NEXT:  2      4     2.00                        smlalbt r5, r6, r4, r1
 # CHECK-NEXT:  2      4     2.00                        smlaltb r4, r2, r3, r2
 # CHECK-NEXT:  2      4     2.00                        smlaltt r8, r3, r8, r4
 # CHECK-NEXT:  2      4     2.00                        smlalbbge r3, r1, r9, r0
 # CHECK-NEXT:  2      4     2.00                        smlalbtle r5, r6, r4, r1
 # CHECK-NEXT:  2      4     2.00                        smlaltbne r4, r2, r3, r2
 # CHECK-NEXT:  2      4     2.00                        smlaltteq r8, r3, r8, r4

 # CHECK-NEXT:  1      2     1.00                        uhsub16 r4, r8, r2
 # CHECK-NEXT:  1      2     1.00                        uhsub16gt r4, r8, r2
 # CHECK-NEXT:  1      2     1.00                        uhsub8  r4, r8, r2
 # CHECK-NEXT:  1      2     1.00                        uhsub8gt  r4, r8, r2
 # CHECK-NEXT:  2      4     2.00                        umaal r3, r4, r5, r6
 # CHECK-NEXT:  2      4     2.00                        umaallt r3, r4, r5, r6
 # CHECK-NEXT:  2      4     2.00                        umlal r2, r4, r6, r8
 # CHECK-NEXT:  2      4     2.00                        umlalgt r6, r1, r2, r6
-# CHECK-NEXT:  2      4     2.00                        umlals  r2, r9, r2, r3
-# CHECK-NEXT:  2      4     2.00                        umlalseq  r3, r5, r1, r2
+# CHECK-NEXT:  4      5     2.00                        umlals  r2, r9, r2, r3
+# CHECK-NEXT:  4      5     2.00                        umlalseq  r3, r5, r1, r2
 # CHECK-NEXT:  2      4     2.00                        umull r2, r4, r6, r8
 # CHECK-NEXT:  2      4     2.00                        umullgt r6, r1, r2, r6
-# CHECK-NEXT:  2      4     2.00                        umulls  r2, r9, r2, r3
-# CHECK-NEXT:  2      4     2.00                        umullseq  r3, r5, r1, r2
+# CHECK-NEXT:  4      5     2.00                        umulls  r2, r9, r2, r3
+# CHECK-NEXT:  4      5     2.00                        umullseq  r3, r5, r1, r2
 # CHECK-NEXT:  1      2     1.00                        uqadd16 r1, r2, r3
 # CHECK-NEXT:  1      2     1.00                        uqadd16gt r4, r7, r9
 # CHECK-NEXT:  1      2     1.00                        uqadd8  r3, r4, r8
 # CHECK-NEXT:  1      2     1.00                        uqadd8le  r8, r1, r2
 # CHECK-NEXT:  2      3     1.00                        uqasx r2, r4, r1
 # CHECK-NEXT:  2      3     1.00                        uqasxhi r5, r2, r9
 # CHECK-NEXT:  2      3     1.00                        uqsax r1, r3, r7
 # CHECK-NEXT:  2      3     1.00                        uqsax r3, r6, r2

 # CHECK-NEXT: [2]   - A57UnitL
 # CHECK-NEXT: [3]   - A57UnitM
 # CHECK-NEXT: [4]   - A57UnitS
 # CHECK-NEXT: [5]   - A57UnitW
 # CHECK-NEXT: [6]   - A57UnitX
 
 # CHECK:      Resource pressure per iteration:
 # CHECK-NEXT: [0]    [1.0]  [1.1]  [2]    [3]    [4]    [5]    [6]
-# CHECK-NEXT: 8.00   133.00 133.00 53.00  522.00 12.00   -      -
+# CHECK-NEXT: 8.00   139.00 139.00 53.00  522.00 12.00   -      -
 
 # CHECK:      Resource pressure by instruction:
 # CHECK-NEXT: [0]    [1.0]  [1.1]  [2]    [3]    [4]    [5]    [6]    Instructions:
 # CHECK-NEXT:  -     0.50   0.50    -      -      -      -      -     adc r1, r2, #15
 # CHECK-NEXT:  -     0.50   0.50    -      -      -      -      -     adc r1, r2, #240
 # CHECK-NEXT:  -     0.50   0.50    -      -      -      -      -     adc r1, r2, #3840
 # CHECK-NEXT:  -     0.50   0.50    -      -      -      -      -     adc r1, r2, #61440
 # CHECK-NEXT:  -     0.50   0.50    -      -      -      -      -     adc r1, r2, #983040

 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     smlabbge  r3, r1, r9, r0
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     smlabtle  r5, r6, r4, r1
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     smlatbne  r4, r2, r3, r2
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     smlatteq  r8, r3, r8, r4
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     smlad r2, r3, r5, r8
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     smladx  r2, r3, r5, r8
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     smladeq r2, r3, r5, r8
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     smladxhi  r2, r3, r5, r8
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlal r2, r3, r5, r8
-# CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlals  r2, r3, r5, r8
+# CHECK-NEXT:  -     1.00   1.00    -     2.00    -      -      -     smlals  r2, r3, r5, r8
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlaleq r2, r3, r5, r8
-# CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlalshi  r2, r3, r5, r8
+# CHECK-NEXT:  -     1.00   1.00    -     2.00    -      -      -     smlalshi  r2, r3, r5, r8
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlalbb r3, r1, r9, r0
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlalbt r5, r6, r4, r1
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlaltb r4, r2, r3, r2
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlaltt r8, r3, r8, r4
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlalbbge r3, r1, r9, r0
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlalbtle r5, r6, r4, r1
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlaltbne r4, r2, r3, r2
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     smlaltteq r8, r3, r8, r4

 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     uhsub16 r4, r8, r2
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     uhsub16gt r4, r8, r2
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     uhsub8  r4, r8, r2
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     uhsub8gt  r4, r8, r2
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     umaal r3, r4, r5, r6
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     umaallt r3, r4, r5, r6
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     umlal r2, r4, r6, r8
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     umlalgt r6, r1, r2, r6
-# CHECK-NEXT:  -      -      -      -     2.00    -      -      -     umlals  r2, r9, r2, r3
-# CHECK-NEXT:  -      -      -      -     2.00    -      -      -     umlalseq  r3, r5, r1, r2
+# CHECK-NEXT:  -     1.00   1.00    -     2.00    -      -      -     umlals  r2, r9, r2, r3
+# CHECK-NEXT:  -     1.00   1.00    -     2.00    -      -      -     umlalseq  r3, r5, r1, r2
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     umull r2, r4, r6, r8
 # CHECK-NEXT:  -      -      -      -     2.00    -      -      -     umullgt r6, r1, r2, r6
-# CHECK-NEXT:  -      -      -      -     2.00    -      -      -     umulls  r2, r9, r2, r3
-# CHECK-NEXT:  -      -      -      -     2.00    -      -      -     umullseq  r3, r5, r1, r2
+# CHECK-NEXT:  -     1.00   1.00    -     2.00    -      -      -     umulls  r2, r9, r2, r3
+# CHECK-NEXT:  -     1.00   1.00    -     2.00    -      -      -     umullseq  r3, r5, r1, r2
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     uqadd16 r1, r2, r3
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     uqadd16gt r4, r7, r9
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     uqadd8  r3, r4, r8
 # CHECK-NEXT:  -      -      -      -     1.00    -      -      -     uqadd8le  r8, r1, r2
 # CHECK-NEXT:  -     0.50   0.50    -     1.00    -      -      -     uqasx r2, r4, r1
 # CHECK-NEXT:  -     0.50   0.50    -     1.00    -      -      -     uqasxhi r5, r2, r9
 # CHECK-NEXT:  -     0.50   0.50    -     1.00    -      -      -     uqsax r1, r3, r7
 # CHECK-NEXT:  -     0.50   0.50    -     1.00    -      -      -     uqsax r3, r6, r2

llvm/utils/TableGen/CodeGenSchedule.cpp

 
 // A Predicate transition is a list of RW sequences guarded by a PredTerm.
 struct PredTransition {
   // A predicate term is a conjunction of PredChecks.
   SmallVector<PredCheck, 4> PredTerm;
   SmallVector<SmallVector<unsigned,4>, 16> WriteSequences;
   SmallVector<SmallVector<unsigned,4>, 16> ReadSequences;
   SmallVector<unsigned, 4> ProcIndices;
+
+  PredTransition() = default;
+  PredTransition(ArrayRef<PredCheck> PT) {
+    PredTerm.assign(PT.begin(), PT.end());
+    ProcIndices.assign(1, 0);
+  }
+  PredTransition(ArrayRef<PredCheck> PT, ArrayRef<unsigned> PIds) {
+    PredTerm.assign(PT.begin(), PT.end());
+    ProcIndices.assign(PIds.begin(), PIds.end());
+  }
 };
 
 // Encapsulate a set of partially constructed transitions.
 // The results are built by repeated calls to substituteVariants.
 class PredTransitions {
   CodeGenSchedModels &SchedModels;
 
 public:
   std::vector<PredTransition> TransVec;
 
   PredTransitions(CodeGenSchedModels &sm): SchedModels(sm) {}
 
   void substituteVariantOperand(const SmallVectorImpl<unsigned> &RWSeq,
-                                bool IsRead, unsigned StartIdx);
+                                bool IsRead, bool IsForAnyCPU,
+                                unsigned StartIdx);
 
   void substituteVariants(const PredTransition &Trans);
 
 #ifndef NDEBUG
   void dump() const;
 #endif
 
 private:
pushVariant(const TransVariant &VInfo, bool IsRead) {
   }
 }
 
 // RWSeq is a sequence of all Reads or all Writes for the next read or write
 // operand. StartIdx is an index into TransVec where partial results
 // starts. RWSeq must be applied to all transitions between StartIdx and the end
 // of TransVec.
 void PredTransitions::substituteVariantOperand(
-  const SmallVectorImpl<unsigned> &RWSeq, bool IsRead, unsigned StartIdx) {
+    const SmallVectorImpl<unsigned> &RWSeq, bool IsRead, bool IsForAnyCPU,
+    unsigned StartIdx) {
+
+  auto CollectAndAddVariants = [&](unsigned TransIdx,
+                                   const CodeGenSchedRW &SchedRW) {
+    // Distribute this partial PredTransition across intersecting variants.
+    // This will push a copies of TransVec[TransIdx] on the back of TransVec.
+    std::vector<TransVariant> IntersectingVariants;
+    getIntersectingVariants(SchedRW, TransIdx, IntersectingVariants);
+    // Now expand each variant on top of its copy of the transition.
+    for (const TransVariant &IV : IntersectingVariants)
+      pushVariant(IV, IsRead);
+    return !IntersectingVariants.empty();
+  };
 
   // Visit each original RW within the current sequence.
   for (SmallVectorImpl<unsigned>::const_iterator
          RWI = RWSeq.begin(), RWE = RWSeq.end(); RWI != RWE; ++RWI) {
     const CodeGenSchedRW &SchedRW = SchedModels.getSchedRW(*RWI, IsRead);
     // Push this RW on all partial PredTransitions or distribute variants.
     // New PredTransitions may be pushed within this loop which should not be
     // revisited (TransEnd must be loop invariant).
+    bool HasAliases = false, WasPushed = false;
     for (unsigned TransIdx = StartIdx, TransEnd = TransVec.size();
          TransIdx != TransEnd; ++TransIdx) {
       // In the common case, push RW onto the current operand's sequence.
       if (!hasAliasedVariants(SchedRW, SchedModels)) {
         if (IsRead)
           TransVec[TransIdx].ReadSequences.back().push_back(*RWI);
         else
           TransVec[TransIdx].WriteSequences.back().push_back(*RWI);
         continue;
       }
-      // Distribute this partial PredTransition across intersecting variants.
-      // This will push a copies of TransVec[TransIdx] on the back of TransVec.
-      std::vector<TransVariant> IntersectingVariants;
-      getIntersectingVariants(SchedRW, TransIdx, IntersectingVariants);
-      // Now expand each variant on top of its copy of the transition.
-      for (std::vector<TransVariant>::const_iterator
-             IVI = IntersectingVariants.begin(),
-             IVE = IntersectingVariants.end();
-           IVI != IVE; ++IVI) {
-        pushVariant(*IVI, IsRead);
-      }
+      HasAliases = true;
+      WasPushed |= CollectAndAddVariants(TransIdx, SchedRW);
+    if (IsRead && IsForAnyCPU && HasAliases && !WasPushed) {
+      // If we're here this means that in some sched class:
+      // a) We have read variant for CPU A
+      // b) We have write variant for CPU B
+      // b) We don't have write variant for CPU A
+      // d) We must expand all read/write variants (IsForAnyCPU is true)
+      // e) We couldn't expand SchedRW because TransVec doesn't have
+      //    any transition with compatible CPU ID.
+      // In such case we create new empty transition with zero (AnyCPU)
+      // index.
+      TransVec.emplace_back(TransVec[StartIdx].PredTerm);
+      TransVec.back().ReadSequences.emplace_back();
+      CollectAndAddVariants(TransVec.size() - 1, SchedRW);
     }
   }
 }
 
 // For each variant of a Read/Write in Trans, substitute the sequence of
 // Read/Writes guarded by the variant. This is exponential in the number of
 // variant Read/Writes, but in practice detection of mutually exclusive
 // predicates should result in linear growth in the total number variants.
 //
 // This is one step in a breadth-first search of nested variants.
 void PredTransitions::substituteVariants(const PredTransition &Trans) {
   // Build up a set of partial results starting at the back of
   // PredTransitions. Remember the first new transition.
   unsigned StartIdx = TransVec.size();
-  TransVec.emplace_back();
-  TransVec.back().PredTerm = Trans.PredTerm;
-  TransVec.back().ProcIndices = Trans.ProcIndices;
+  TransVec.emplace_back(Trans.PredTerm, Trans.ProcIndices);
 
+  bool IsForAnyCPU = llvm::count(Trans.ProcIndices, 0);
   // Visit each original write sequence.
   for (SmallVectorImpl<SmallVector<unsigned,4>>::const_iterator
          WSI = Trans.WriteSequences.begin(), WSE = Trans.WriteSequences.end();
        WSI != WSE; ++WSI) {
     // Push a new (empty) write sequence onto all partial Transitions.
     for (std::vector<PredTransition>::iterator I =
            TransVec.begin() + StartIdx, E = TransVec.end(); I != E; ++I) {
       I->WriteSequences.emplace_back();
     }
-    substituteVariantOperand(*WSI, /*IsRead=*/false, StartIdx);
+    substituteVariantOperand(*WSI, /*IsRead=*/false, IsForAnyCPU, StartIdx);
   }
   // Visit each original read sequence.
   for (SmallVectorImpl<SmallVector<unsigned,4>>::const_iterator
          RSI = Trans.ReadSequences.begin(), RSE = Trans.ReadSequences.end();
        RSI != RSE; ++RSI) {
     // Push a new (empty) read sequence onto all partial Transitions.
     for (std::vector<PredTransition>::iterator I =
            TransVec.begin() + StartIdx, E = TransVec.end(); I != E; ++I) {
       I->ReadSequences.emplace_back();
     }
-    substituteVariantOperand(*RSI, /*IsRead=*/true, StartIdx);
+    substituteVariantOperand(*RSI, /*IsRead=*/true, IsForAnyCPU, StartIdx);
   }
 }
 
 static void addSequences(CodeGenSchedModels &SchedModels,
                          const SmallVectorImpl<SmallVector<unsigned, 4>> &Seqs,
                          IdxVec &Result, bool IsRead) {
   for (const auto &S : Seqs)
     if (!S.empty())
       Result.push_back(SchedModels.findOrInsertRW(S, IsRead));
 }
 
 static void dumpTransition(const CodeGenSchedModels &SchedModels,