[PATCH] D49243: [X86] Improved sched models for X86 BT*rr instructions

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a subscriber: gchatelet.
RKSimon added inline comments.


================
Comment at: lib/Target/X86/X86SchedHaswell.td:632
 }
 def : InstRW<[HWWriteBTRSCmr], (instregex "BT(R|S|C)(16|32|64)mr")>;
 
----------------
@craig.topper @courbet @gchatelet  These look completely wrong (and BTmr above) - and Broadwell appears to be missing them as well - any suggestions for the bit tests memory cases?


================
Comment at: lib/Target/X86/X86Schedule.td:122
+// Bit Test
+defm WriteBTr    : X86SchedWritePair;
+
----------------
avt77 wrote:
> lebedev.ri wrote:
> > RKSimon wrote:
> > > lebedev.ri wrote:
> > > > Hmm. Nits:
> > > > 1. (not a nit) The suffix `r` notes that only the non-mem versions are covered.
> > > >    I wonder if we can convey that somehow better.
> > > > 2. These cover 4 different bit-test instructions - `bt`,`bt[rcs]`
> > > >    Naming this `WriteBTr` //may// be confizing - is this only about `bt` instruction?
> > > >    How about calling it `WriteBitTest`?
> > > I'm confused - this should be probably be called WriteBT. But then you've declared this as a X86SchedWritePair but you're not using the folded half of the pair?
> > Note that it *only* covers `rr` versions, and does not include `mr` versions.
> > So yeah, maybe it shouldn't be `X86SchedWritePair`, but `X86WriteRes`?
> I'm going to implement mr version asap that's why I use Pair here.
If the memory cases are causing a problem it'd be acceptable to just do a reg-reg version for now:

def  WriteBitTest    : SchedWrite // Bit Test - TODO add memory folding support

And you can come back to the memory cases once we understand whats to be done. I just don't want a X86SchedWritePair def when you're not using the folded case.


https://reviews.llvm.org/D49243