Page MenuHomePhabricator

[PPC] Handle weighted comparisons when inserting selects.
ClosedPublic

Authored by iteratee on Jan 7 2016, 6:12 PM.

Details

Reviewers
echristo
hfinkel
Summary

Only non-weighted predicates were handled in PPCInstrInfo::insertSelect. Handle the weighted predicates as well.

This latent bug was triggered by r255398, because it added use of the branch-weighted predicates.

While here, switch over an enum instead of an int to get the compiler to enforce totality in the future.

Diff Detail

Event Timeline

iteratee updated this revision to Diff 44303.Jan 7 2016, 6:12 PM
iteratee retitled this revision from to [PPC] Handle weighted comparisons when inserting selects..
iteratee updated this object.
iteratee added a reviewer: hfinkel.
iteratee added a subscriber: llvm-commits.
echristo added a subscriber: echristo.

I think you can trim down the lists of attributes in the testcase, also needs a more descriptive description of what the bug is and how this is fixing it - mostly, can you elaborate more? I know what it's fixing, but for the commit it's better to be more descriptive.

Thanks!

-eric

hfinkel added inline comments.Jan 8 2016, 12:13 PM
lib/Target/PowerPC/PPCInstrInfo.cpp
747

On architectures where isel is not always cheaper than the branch sequence, we need to be careful here. In fact, Kit is already working on code to expand isels late into branches to help the branch predictor do the right thing on P7/P8/etc. Enabling this conversion is good for some cores where isel really is cheap (on the PPC A2 for example), but I highly suspect that for P7/P8/etc. we should just reject the transformation entirely as to not lose the hinting information.

Thus, please either add code (or a FIXME) in canInsertSelect that we should not be converting hinted branches into isels on the P7/P8/etc.

I think the cleanest way to do all this might be to:

  1. Implement a function that gives the non-hinted predicate from a hinted predicate, say, getNonHintedPredicate. Then, here, we just need:

    switch(getNonHintedPredicate(SelectPred)) {

and in canInsertSelect, we can have:

unsigned Directive =
  DAG->MF.getSubtarget<PPCSubtarget>().getDarwinDirective();
unsigned SelectPred = Cond[0].getImm();
// Don't convert hinted branches into isel on the P7, and similar because losing the hinting information is likely worse than any benefit the isel brings.
if (getNonHintedPredicate(SelectPred) != SelectPred && (Directive == PPC::DIR_PWR7 || Directive == PPC::DIR_PWR8))
  return false;

I've cleaned up the unneeded attributes from the testcase, and will upload them shortly.

lib/Target/PowerPC/PPCInstrInfo.cpp
747

I agree that we should only perform this when it's a performance win, but I don't like the rest of the approach.
I think the switch should cover all cases and use an enum, as this bug would have been caught in that case.
For canInsertSelect, I think isHintedPredicate(SelectPred) is much cleaner as well.

I'm not sure that using a list of processor directives is right either. Can you suggest a processor feature that we can add to make this happen, like I did with HasFusion? This reduces the maintenance burden of adding a new subtarget.

hfinkel added inline comments.Jan 8 2016, 3:10 PM
lib/Target/PowerPC/PPCInstrInfo.cpp
747

I'm not sure that using a list of processor directives is right either. Can you suggest a processor feature that we can add to make this happen, like I did with HasFusion? This reduces the maintenance burden of adding a new subtarget.

Good point. I'm fine with HasSlowISEL (to go along with our HasISEL). Thanks!

iteratee updated this object.Jan 8 2016, 3:56 PM
iteratee updated this revision to Diff 44549.Jan 11 2016, 1:57 PM

I have patches for the requested fixes out. Are there any other issues on this patch?

echristo accepted this revision.Jan 11 2016, 2:17 PM
echristo edited edge metadata.

Not from me, the testcase looks fine here with the one nit I had.

Go ahead and wait for Hal first please :)

-eric

test/CodeGen/PowerPC/2016-01-07-BranchWeightCrash.ll
34

Not used :)

This revision is now accepted and ready to land.Jan 11 2016, 2:17 PM
iteratee updated this revision to Diff 44558.Jan 11 2016, 2:19 PM
iteratee edited edge metadata.

remove unused #0 attributes.

iteratee marked 4 inline comments as done.Jan 12 2016, 12:44 PM
iteratee closed this revision.Jan 12 2016, 1:05 PM
hfinkel edited edge metadata.Jan 12 2016, 5:10 PM

LGTM too, thanks!