Page MenuHomePhabricator

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

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

Details

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
evgeny777 requested review of this revision.Oct 20 2020, 2:17 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.

Can you rebase and upload with context?

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.

@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