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

Thu May 30 09:00:46 PDT 2019

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

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

https://reviews.llvm.org/D62164