[PATCH] D90844: [TableGen][SchedModels] Fix read/write variant substitution #2

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Hello. Sorry for the delay. I don't know this code super well and was trying to figure out what the different parts were doing. I see that the produced output has code like this:

  case 703: // t2ADDSrs
    if (SchedModel->getProcessorID() == 4) { // CortexA57Model
      if ((TII->isSwiftFastImmShift(MI))
          && TII->isPredicated(*MI))
        return 1066; // _ReadDefault
    }

The isSwiftFastImmShift is odd, when paired with a CortexA57Model, and comes from using StartIdx's PredTerm in `TransVec.emplace_back(TransVec[StartIdx].PredTerm)` I think. The _ReadDefault as the schedule class also sounds odd to me, as opposed to something that would use Writes too.



================
Comment at: llvm/lib/Target/ARM/ARMScheduleA57.td:180
 // May be an error in doc.
-def A57WriteALUsi : SchedWriteVariant<[
-  // lsl #2, lsl #1, or lsr #1.
----------------
evgeny777 wrote:
> dmgreen wrote:
> > evgeny777 wrote:
> > > dmgreen wrote:
> > > > This is "Move, shift by immed, no setflags"  _and_  "Move, shift by immed, setflags"?
> > > > I agree that the predicated pred should not matter, but there probably should be some difference between flag setting and not.
> > > > 
> > > > I think the TODO above is referring to A57WriteALUSsr? I'm not sure why A57WriteALUsr is treated the same way though. From what I can see it should be using  A57Write_1cyc_1.
> > > > 
> > > > Is A57ReadALUsr worth keeping around?
> > > > Is A57ReadALUsr worth keeping around?
> > > 
> > > I don't think it is, however I decided to keep it for now for testing purposes.
> > > 
> > > >  I'm not sure why A57WriteALUsr is treated the same way though. From what I can see it should be using A57Write_1cyc_1.
> > > Why? From opt guide:
> > > ```
> > > ALU, shift	by register,	
> > > unconditional                          (same as above) 2 1 M
> > > ALU, shift by register,	
> > > conditional                            (same as above) 2 1 I0/I1
> > > ```
> > Hmm. Which opt guide is that from? I seem to see:
> >   Move, shift by immed, no setflags    1I
> >   Move, shift by immed, setflags    2M
> >   Move, shift by register, no setflags, unconditional    1I
> >   Move, shift by register, no setflags, conditional    2I
> >   Move, shift by register, setflags, unconditional    2M
> >   Move, shift by register, setflags, conditional    2I
> > 
> > So the first 2 are currently in WriteALUsi (which should probably be split up to get it correct, but that is a separate issue.  A better default is probably A57Write_1cyc_1I).
> > A57WriteALUSsr is the last 2 and fits the opt guide correctly at least.
> > A57WriteALUsr is the middle two, but should probably be using Pred:A57Write_2cyc_1I and NoPred: A57Write_1cyc_1I.
> > 
> > Hmm. Which opt guide is that from? 
> 
> I think we're both looking at the same one. I've copy-pasted from section 3.3
> 
> > I seem to see:
> 
> Right, but `Move, shift by register` (MOVsr) is bound to `WriteALU` (ARM version) and thumb version (t2MOVsr) is unbound. The following commands are currently bound to WriteALUsr:
> 
> ```
> ADCrsr ADDrsr ANDrsr BICrsr EORrsr ORRrsr RSBrsr RSCrsr SBCrsr SUBrsr SXTAB SXTAB16 SXTAH UXTAB UXTAB16 UXTAH
> ```
> It seems ARM/Thumb instruction definition is incomplete and broken in many ways when it comes to scheduling. Patch however is more about simplifying adding new models, not fixing existing ones.
Ah. I was thinking more about shifts than arithmetic operations. In that case, A57Write_2cyc_1M is probably a better default than A57Write_1cyc_1I.


================
Comment at: llvm/utils/TableGen/CodeGenSchedule.cpp:1362
 
+template <> struct llvm::DenseMapInfo<TransVariant> {
+  static inline TransVariant getEmptyKey() { return {nullptr, 0, 0, 0}; }
----------------
Does this need to use a densemap? It seems to be being used to check whether the TransVariant have already been handled. Can it use a set or something simpler for that?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90844/new/

https://reviews.llvm.org/D90844