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

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


================
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.
----------------
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.


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

https://reviews.llvm.org/D90844