This is an archive of the discontinued LLVM Phabricator instance.

Differential D89777

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

Authored by evgeny777 on Oct 20 2020, 2:17 AM.

Details

Reviewers
SjoerdMeijer
javed.absar
dmgreen
andreadb
Commits
rGcc96a822917c: [TableGen][SchedModels] Fix read/write variant substitution
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.

Diff Detail

Event Timeline

evgeny777 created this revision.Oct 20 2020, 2:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2020, 2:17 AM
Herald added subscribers: gbedwell, hiraditya. · View Herald Transcript
evgeny777 requested review of this revision.Oct 20 2020, 2:17 AM
dmgreen added a comment.Oct 22 2020, 8:16 AM

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 added inline comments.Oct 22 2020, 8:18 AM
llvm/test/tools/llvm-mca/ARM/cortex-a57-basic-instructions.s
2287
def A57Write_4cyc_1M  : SchedWriteRes<[A57UnitL]> { let Latency = 4;  }

Likely.

evgeny777 mentioned this in D90152: Fix issue in cortex-a57 sched model.Oct 26 2020, 5:24 AM
evgeny777 added a comment.Oct 29 2020, 1:18 AM

Ping

dmgreen added a comment.Oct 29 2020, 1:22 AM

Can you rebase and upload with context?

evgeny777 updated this revision to Diff 301538.Oct 29 2020, 1:59 AM

Addressed

dmgreen added a comment.Oct 29 2020, 4:09 AM

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
1653

I feel like we might need to do this.

evgeny777 updated this revision to Diff 301979.Oct 30 2020, 11:52 AM

@dmgreen Nice find! I've fixed it

dmgreen accepted this revision.Nov 2 2020, 2:19 AM

LGTM. Thanks for the update.

llvm/utils/TableGen/CodeGenSchedule.cpp
1592

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

This revision is now accepted and ready to land.Nov 2 2020, 2:19 AM
Closed by commit rGcc96a822917c: [TableGen][SchedModels] Fix read/write variant substitution (authored by evgeny777). · Explain WhyNov 2 2020, 6:39 AM
This revision was automatically updated to reflect the committed changes.
evgeny777 added a commit: rGcc96a822917c: [TableGen][SchedModels] Fix read/write variant substitution.
evgeny777 mentioned this in D90844: [TableGen][SchedModels] Fix read/write variant substitution #2.Nov 5 2020, 5:55 AM

Revision Contents

PathSize
llvm/
lib/
Target/
ARM/
6 lines
test/
tools/
llvm-mca/
ARM/
66 lines
utils/
TableGen/
28 lines

Diff 299297

llvm/lib/Target/ARM/ARMScheduleA57.td

Context not available.
// from similar μops, allowing a typical sequence of multiply-accumulate μops // from similar μops, allowing a typical sequence of multiply-accumulate μops
// to issue one every 1 cycle (sched advance = 2). // to issue one every 1 cycle (sched advance = 2).
def A57WriteMLA : SchedWriteRes<[A57UnitM]> { let Latency = 3; } 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 A57ReadMLA : SchedReadAdvance<2, [A57WriteMLA, A57WriteMLAL]>;
def : InstRW<[A57WriteMLA], def : InstRW<[A57WriteMLA],
Context not available.

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

Context not available.
# CHECK-NEXT: 1 3 1.00 smladeq 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: 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 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 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 smlalbb r3, r1, r9, r0
# CHECK-NEXT: 2 4 2.00 smlalbt r5, r6, r4, r1 # 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 smlaltb r4, r2, r3, r2
Context not available.
# CHECK-NEXT: 2 4 2.00 umaallt 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 umlal r2, r4, r6, r8
# CHECK-NEXT: 2 4 2.00 umlalgt r6, r1, r2, r6 # CHECK-NEXT: 2 4 2.00 umlalgt r6, r1, r2, r6
# CHECK-NEXT: 2 4 2.00 umlals r2, r9, r2, r3 # CHECK-NEXT: 4 5 2.00 umlals r2, r9, r2, r3
# CHECK-NEXT: 2 4 2.00 umlalseq r3, r5, r1, r2 # 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 umull r2, r4, r6, r8
# CHECK-NEXT: 2 4 2.00 umullgt r6, r1, r2, r6 # CHECK-NEXT: 2 4 2.00 umullgt r6, r1, r2, r6
# CHECK-NEXT: 2 4 2.00 umulls r2, r9, r2, r3 # CHECK-NEXT: 4 5 2.00 umulls r2, r9, r2, r3
# CHECK-NEXT: 2 4 2.00 umullseq r3, r5, r1, r2 # 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 uqadd16 r1, r2, r3
# CHECK-NEXT: 1 2 1.00 uqadd16gt r4, r7, r9 # 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 uqadd8 r3, r4, r8
Context not available.
# CHECK: Resource pressure per iteration: # CHECK: Resource pressure per iteration:
# CHECK-NEXT: [0] [1.0] [1.1] [2] [3] [4] [5] [6] # CHECK-NEXT: [0] [1.0] [1.1] [2] [3] [4] [5] [6]
# CHECK-NEXT: 8.00 159.00 159.00 171.00 496.00 12.00 - - # CHECK-NEXT: 8.00 165.00 165.00 211.00 456.00 12.00 - -
# CHECK: Resource pressure by instruction: # CHECK: Resource pressure by instruction:
# CHECK-NEXT: [0] [1.0] [1.1] [2] [3] [4] [5] [6] Instructions: # CHECK-NEXT: [0] [1.0] [1.1] [2] [3] [4] [5] [6] Instructions:
Context not available.
# CHECK-NEXT: - - - - 1.00 - - - smladx 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 - - - smladeq r2, r3, r5, r8
# CHECK-NEXT: - - - - 1.00 - - - smladxhi 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 - - - - smlal r2, r3, r5, r8
dmgreenUnsubmitted
Not Done
ReplyInline Actions

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

dmgreen: These now use L somehow? Am I reading that right?
dmgreenUnsubmitted
Not Done
ReplyInline Actions

Oh. Is A57Write_4cyc_1M incorrectly specified?

dmgreen: Oh. Is A57Write_4cyc_1M incorrectly specified?
evgeny777AuthorUnsubmitted
Done
ReplyInline Actions
def A57Write_4cyc_1M  : SchedWriteRes<[A57UnitL]> { let Latency = 4;  }

Likely.

evgeny777: ``` def A57Write_4cyc_1M : SchedWriteRes<[A57UnitL]> { let Latency = 4; } ``` Likely.
# 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 - - - - 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 - - - - smlalbb r3, r1, r9, r0
# CHECK-NEXT: - - - - 2.00 - - - smlalbt r5, r6, r4, r1 # CHECK-NEXT: - - - 2.00 - - - - smlalbt r5, r6, r4, r1
# CHECK-NEXT: - - - - 2.00 - - - smlaltb r4, r2, r3, r2 # CHECK-NEXT: - - - 2.00 - - - - smlaltb r4, r2, r3, r2
# CHECK-NEXT: - - - - 2.00 - - - smlaltt r8, r3, r8, r4 # CHECK-NEXT: - - - 2.00 - - - - smlaltt r8, r3, r8, r4
# CHECK-NEXT: - - - - 2.00 - - - smlalbbge r3, r1, r9, r0 # CHECK-NEXT: - - - 2.00 - - - - smlalbbge r3, r1, r9, r0
# CHECK-NEXT: - - - - 2.00 - - - smlalbtle r5, r6, r4, r1 # CHECK-NEXT: - - - 2.00 - - - - smlalbtle r5, r6, r4, r1
# CHECK-NEXT: - - - - 2.00 - - - smlaltbne r4, r2, r3, r2 # CHECK-NEXT: - - - 2.00 - - - - smlaltbne r4, r2, r3, r2
# CHECK-NEXT: - - - - 2.00 - - - smlaltteq r8, r3, r8, r4 # CHECK-NEXT: - - - 2.00 - - - - smlaltteq r8, r3, r8, r4
# CHECK-NEXT: - - - - 2.00 - - - smlald r2, r3, r5, r8 # CHECK-NEXT: - - - 2.00 - - - - smlald r2, r3, r5, r8
# CHECK-NEXT: - - - 2.00 - - - - smlaldx r2, r3, r5, r8 # CHECK-NEXT: - - - 2.00 - - - - smlaldx r2, r3, r5, r8
# CHECK-NEXT: - - - - 2.00 - - - smlaldeq r2, r3, r5, r8 # CHECK-NEXT: - - - 2.00 - - - - smlaldeq r2, r3, r5, r8
# CHECK-NEXT: - - - 2.00 - - - - smlaldxhi r2, r3, r5, r8 # CHECK-NEXT: - - - 2.00 - - - - smlaldxhi r2, r3, r5, r8
# CHECK-NEXT: - - - - 1.00 - - - smlawb r2, r3, r10, r8 # CHECK-NEXT: - - - - 1.00 - - - smlawb r2, r3, r10, r8
# CHECK-NEXT: - - - - 1.00 - - - smlawt r8, r3, r5, r9 # CHECK-NEXT: - - - - 1.00 - - - smlawt r8, r3, r5, r9
Context not available.
# CHECK-NEXT: - - - - 1.00 - - - smlsdx r2, r3, r5, r8 # CHECK-NEXT: - - - - 1.00 - - - smlsdx r2, r3, r5, r8
# CHECK-NEXT: - - - - 1.00 - - - smlsdeq r2, r3, r5, r8 # CHECK-NEXT: - - - - 1.00 - - - smlsdeq r2, r3, r5, r8
# CHECK-NEXT: - - - - 1.00 - - - smlsdxhi r2, r3, r5, r8 # CHECK-NEXT: - - - - 1.00 - - - smlsdxhi r2, r3, r5, r8
# CHECK-NEXT: - - - - 2.00 - - - smlsld r2, r9, r5, r1 # CHECK-NEXT: - - - 2.00 - - - - smlsld r2, r9, r5, r1
# CHECK-NEXT: - - - 2.00 - - - - smlsldx r4, r11, r2, r8 # CHECK-NEXT: - - - 2.00 - - - - smlsldx r4, r11, r2, r8
# CHECK-NEXT: - - - - 2.00 - - - smlsldeq r8, r2, r5, r6 # CHECK-NEXT: - - - 2.00 - - - - smlsldeq r8, r2, r5, r6
# CHECK-NEXT: - - - 2.00 - - - - smlsldxhi r1, r0, r3, r8 # CHECK-NEXT: - - - 2.00 - - - - smlsldxhi r1, r0, r3, r8
# CHECK-NEXT: - - - - 1.00 - - - smmla r1, r2, r3, r4 # CHECK-NEXT: - - - - 1.00 - - - smmla r1, r2, r3, r4
# CHECK-NEXT: - - - - 1.00 - - - smmlar r4, r3, r2, r1 # CHECK-NEXT: - - - - 1.00 - - - smmlar r4, r3, r2, r1
Context not available.
# CHECK-NEXT: - - - - 1.00 - - - uhsub16gt r4, r8, r2 # CHECK-NEXT: - - - - 1.00 - - - uhsub16gt r4, r8, r2
# CHECK-NEXT: - - - - 1.00 - - - uhsub8 r4, r8, r2 # CHECK-NEXT: - - - - 1.00 - - - uhsub8 r4, r8, r2
# CHECK-NEXT: - - - - 1.00 - - - uhsub8gt r4, r8, r2 # CHECK-NEXT: - - - - 1.00 - - - uhsub8gt r4, r8, r2
# CHECK-NEXT: - - - - 2.00 - - - umaal r3, r4, r5, r6 # CHECK-NEXT: - - - 2.00 - - - - umaal r3, r4, r5, r6
# CHECK-NEXT: - - - - 2.00 - - - umaallt 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 - - - - umlal r2, r4, r6, r8
# CHECK-NEXT: - - - - 2.00 - - - umlalgt r6, r1, r2, r6 # CHECK-NEXT: - - - 2.00 - - - - umlalgt r6, r1, r2, r6
# CHECK-NEXT: - - - - 2.00 - - - umlals r2, r9, r2, r3 # CHECK-NEXT: - 1.00 1.00 - 2.00 - - - umlals r2, r9, r2, r3
# CHECK-NEXT: - - - - 2.00 - - - umlalseq r3, r5, r1, r2 # 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 - - - - umull r2, r4, r6, r8
# CHECK-NEXT: - - - - 2.00 - - - umullgt r6, r1, r2, r6 # CHECK-NEXT: - - - 2.00 - - - - umullgt r6, r1, r2, r6
# CHECK-NEXT: - - - - 2.00 - - - umulls r2, r9, r2, r3 # CHECK-NEXT: - 1.00 1.00 - 2.00 - - - umulls r2, r9, r2, r3
# CHECK-NEXT: - - - - 2.00 - - - umullseq r3, r5, r1, r2 # 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 - - - uqadd16 r1, r2, r3
# CHECK-NEXT: - - - - 1.00 - - - uqadd16gt r4, r7, r9 # CHECK-NEXT: - - - - 1.00 - - - uqadd16gt r4, r7, r9
# CHECK-NEXT: - - - - 1.00 - - - uqadd8 r3, r4, r8 # CHECK-NEXT: - - - - 1.00 - - - uqadd8 r3, r4, r8
Context not available.

llvm/utils/TableGen/CodeGenSchedule.cpp

Context not available.
// //
// This is one step in a breadth-first search of nested variants. // This is one step in a breadth-first search of nested variants.
void PredTransitions::substituteVariants(const PredTransition &Trans) { void PredTransitions::substituteVariants(const PredTransition &Trans) {
auto NewTrans = [&]() {
unsigned I = TransVec.size();
TransVec.emplace_back();
TransVec.back().PredTerm = Trans.PredTerm;
TransVec.back().ProcIndices = Trans.ProcIndices;
return I;
};
// Build up a set of partial results starting at the back of // Build up a set of partial results starting at the back of
// PredTransitions. Remember the first new transition. // PredTransitions. Remember the first new transition.
unsigned StartIdx = TransVec.size(); unsigned StartIdx = NewTrans();
TransVec.emplace_back();
TransVec.back().PredTerm = Trans.PredTerm;
TransVec.back().ProcIndices = Trans.ProcIndices;
// Visit each original write sequence. // Visit each original write sequence.
for (SmallVectorImpl<SmallVector<unsigned,4>>::const_iterator for (SmallVectorImpl<SmallVector<unsigned,4>>::const_iterator
WSI = Trans.WriteSequences.begin(), WSE = Trans.WriteSequences.end(); WSI = Trans.WriteSequences.begin(), WSE = Trans.WriteSequences.end();
Context not available.
} }
substituteVariantOperand(*WSI, /*IsRead=*/false, StartIdx); substituteVariantOperand(*WSI, /*IsRead=*/false, StartIdx);
} }
// We may have both read and write variant in the same sched class.
// If we've previously craeted valid write variant transition then create
// new empty transition for read variant, because read and write transition
// may belong to different processor models.
if (!Trans.ProcIndices[0] && TransVec[StartIdx].ProcIndices[0])
StartIdx = NewTrans();
// Visit each original read sequence. // Visit each original read sequence.
for (SmallVectorImpl<SmallVector<unsigned,4>>::const_iterator for (SmallVectorImpl<SmallVector<unsigned,4>>::const_iterator
RSI = Trans.ReadSequences.begin(), RSE = Trans.ReadSequences.end(); RSI = Trans.ReadSequences.begin(), RSE = Trans.ReadSequences.end();
Context not available.
} }
substituteVariantOperand(*RSI, /*IsRead=*/true, StartIdx); substituteVariantOperand(*RSI, /*IsRead=*/true, StartIdx);
} }
// If we haven't substituted operand then just remove previously
// craeted empty transition.
// TODO: Merge read and write transitions if they share the same
dmgreenUnsubmitted
Not Done
ReplyInline Actions

I feel like we might need to do this.

dmgreen: I feel like we might need to do this.
// processor model.
if (!TransVec[StartIdx].ProcIndices[0]) {
assert(StartIdx == TransVec.size() - 1);
TransVec.pop_back();
}
} }
static void addSequences(CodeGenSchedModels &SchedModels, static void addSequences(CodeGenSchedModels &SchedModels,
Context not available.