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

[Dave Green via Phabricator via llvm-commits]

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



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

https://reviews.llvm.org/D90844