Page MenuHomePhabricator

[PowerPC] Enable MachinePipeliner for P9 with -ppc-enable-pipeliner
ClosedPublic

Authored by jsji on May 20 2019, 2:55 PM.

Details

Summary

Implement necessary target hooks to enable MachinePipeliner for P9 only.
The pass is off on PowerPC by def ault,
can be enabled with -ppc-enable-pipeliner for P9.

Diff Detail

Event Timeline

jsji created this revision.May 20 2019, 2:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 2:55 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
jsji added a comment.May 28 2019, 8:10 PM

Ping.. The machine independent part has been committed. Thanks. @bcahoon

Any comments and feedback to this PowerPC specific part are appreciated. @hfinkel @nemanjai @kbarton @steven.zhang Thanks.

hfinkel added inline comments.May 28 2019, 8:38 PM
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3938

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?

3962

We generally don't directly recurse though the CFG, but use a worklist and a loop. Can you do that here?

4003

Extra space before 'to'.

4007

Period at the end of the comment.

llvm/lib/Target/PowerPC/PPCSubtarget.cpp
192

Given that this is off by default anyway, why is it enabled only for the P9?

jsji marked an inline comment as done.May 28 2019, 9:16 PM
jsji added inline comments.
llvm/lib/Target/PowerPC/PPCSubtarget.cpp
192

It is mainly because only P9 has InstrSchedModel , P8 and below are still using Itineraries.
And it needs additional non trivial effort to enable DFA support for P8 and below, due to DFA limitation.
Considering our future process will use InstrSchedModel, we chose to enable InstrSchedModel first for now.

jsji updated this revision to Diff 202043.May 29 2019, 1:24 PM
jsji marked 3 inline comments as done.

Use worklist instead, and fix comments.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3938

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.

3962

Sure.

hfinkel added inline comments.May 29 2019, 2:00 PM
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3938

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

jsji marked an inline comment as done.May 30 2019, 9:00 AM
jsji added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3938

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.

jsji marked an inline comment as done.Jun 3 2019, 12:37 PM
jsji added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3938

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?

hfinkel added inline comments.Jun 3 2019, 12:53 PM
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3938

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.

jsji updated this revision to Diff 202847.Jun 3 2019, 8:18 PM

Update TII interface to pass down preheader, and find mtctr in preheader only.

jsji marked 2 inline comments as done.Jun 3 2019, 8:20 PM
jsji added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3938

Updated patch to just check in preheader.

hfinkel added inline comments.Jun 4 2019, 8:49 AM
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3947

Two things:

  1. Never use assert(0 && ...), that's what llvm_unreachable is for.
  2. Don't assert here, just return nullptr. This is not actually an error, it *could* happen, and the compiler shouldn't crash. If you'd like, add a STATISTIC to count the number of times this happens.
jsji updated this revision to Diff 202980.Jun 4 2019, 10:32 AM
jsji marked an inline comment as done.

Remove the assert(0).

jsji marked 2 inline comments as done.Jun 4 2019, 10:33 AM
jsji added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3947

You are right, I should have removed this before posting it. Thanks.

hfinkel added inline comments.Jun 4 2019, 11:41 AM
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3978

This return value is incorrect if Offset was zero?

jsji updated this revision to Diff 202995.Jun 4 2019, 12:12 PM
jsji marked an inline comment as done.

Update return code for Zero count.

jsji marked an inline comment as done.Jun 4 2019, 12:13 PM

Thanks for catching this.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3978

Good catch. Although we shouldn't call reduceLoopCount with trip count == 0, this should be fixed.

jsji added a comment.Jun 11 2019, 8:34 AM

Ping... Any further comments and feedback? @hfinkel . Thanks!

hfinkel accepted this revision.Jun 11 2019, 8:48 AM

LGTM

This revision is now accepted and ready to land.Jun 11 2019, 8:48 AM
This revision was automatically updated to reflect the committed changes.

Hi,

This patch seems to fail under Asan at https://github.com/llvm-mirror/llvm/blob/28bea3dbfef348e53cf48f921e96a35b642b3950/lib/CodeGen/MachinePipeliner.cpp#L3736.

I verified that lower_bound() may return Indices.end() upon running the test CodeGen/PowerPC/sms-simple.ll.

jsji added a comment.Jun 11 2019, 4:16 PM

Hi,

This patch seems to fail under Asan at https://github.com/llvm-mirror/llvm/blob/28bea3dbfef348e53cf48f921e96a35b642b3950/lib/CodeGen/MachinePipeliner.cpp#L3736.

I verified that lower_bound() may return Indices.end() upon running the test CodeGen/PowerPC/sms-simple.ll.

Thanks, yes, it looks like an issue exposed by power enablement. I have removed sms-simple.ll temporarily to unblock build bot. I will investigate and fix the root cause.

jsji added a comment.Jun 13 2019, 10:13 AM

Thanks, yes, it looks like an issue exposed by power enablement. I have removed sms-simple.ll temporarily to unblock build bot. I will investigate and fix the root cause.

https://reviews.llvm.org/D63282 created for the fix.