[PATCH] D62164: [PowerPC] Enable MachinePipeliner for P9 with -ppc-enable-pipeliner

[Jinsong Ji via Phabricator via llvm-commits]

jsji marked 2 inline comments as done.
jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3936
+
+MachineInstr *PPCInstrInfo::findLoopInstr(
+    const MachineBasicBlock &BB, unsigned EndLoopOp,
----------------
hfinkel wrote:
> jsji wrote:
> > jsji wrote:
> > > hfinkel wrote:
> > > > jsji wrote:
> > > > > hfinkel wrote:
> > > > > > FWIW, I find this method a bit strange. Do we need have a MachineLoopInfo available such that we could just check in the preheader of the loop?
> > > > > In short, yes. 
> > > > > MachinePipeliner have MachineLoopInfo, and it also make sure that we call `reduceLoopCount` with prolog MBBs that set up the pipeline, so we are safe to assume that `the loop set-up instruction will be in a predecessor block` only.
> > > > I'd not give it a 100% guarantee, but if you only check in the preheader, do you miss anything?
> > > > 
> > > > (you could check, perhaps, by putting an assert in the current logic to see if you every find the mtctr in some block other than the preheader).
> > > Oh, sorry, did not read carefully, the 'yes' in my last comment was for 'predecessors' not 'preheader' - we assume that mtctr will be in predecessors of Prolog MBBs, no necessary in preheader only.
> > > 
> > > I guess you question is about whether we may miss some opportunity here - can NOT find mtctr in some case when mtctr is NOT in predecessors?
> > > 
> > > From the code in MachinePipeliner, I think we should not missing anything here. But I will try your suggestion to double confirm.
> > > you could check, perhaps, by putting an assert in the current logic to see if you ever find the mtctr in some block other than the preheader
> > 
> > Added asserts before `return nullptr`, and run test with testsuites, no assert found. 
> > Also added some MBB dump for all the BBs we found mtctr, and check all the MBBs that contains mtctr (5000+ mtctr found in testsuites), all of them are in preheader, nothing special found.
> > 
> > 
> > I also look into `PPCCTRLoops` where we generate mtctr, and looks like that we will only `Insert the count into the preheader `, and PPCCTRLoopsVerify also trying to find `MTCTR8loop`/`MTCTRloop` in predecessors only.
> > 
> > I am not aware of any following passes that might move mtctr to other places.
> > 
> > @hfinkel Do you have any cases in mind that I should check again? 
> > 
> Nope. That's what I suspected that you might find. I think that just checking the preheader, should it exist, will be sufficient here in practice. I recommend doing that, and then if we find cases where we need to do more, revisit at that time.
Updated patch to just check in preheader. 


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

https://reviews.llvm.org/D62164