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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3936
+
+MachineInstr *PPCInstrInfo::findLoopInstr(
+    const MachineBasicBlock &BB, unsigned EndLoopOp,
----------------
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.


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

https://reviews.llvm.org/D62164