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

[Jinsong Ji via Phabricator via llvm-commits]

jsji marked an inline comment as done.
jsji added inline comments.


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



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

https://reviews.llvm.org/D62164