This is an archive of the discontinued LLVM Phabricator instance.

[AggressiveInstCombine] Generalize foldGuardedRotateToFunnelShift to generic funnel shifts
ClosedPublic

Authored by RKSimon on Nov 2 2020, 9:42 AM.

Details

Summary

The fold currently only handles rotation patterns, but with the maturation of backend funnel shift handling we can now realistically handle all funnel shift patterns.

This should allow us to begin resolving PR46896 et al.

Diff Detail

Event Timeline

RKSimon created this revision.Nov 2 2020, 9:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2020, 9:42 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon requested review of this revision.Nov 2 2020, 9:42 AM
RKSimon added inline comments.
llvm/test/Transforms/AggressiveInstCombine/funnel.ll
430

This does fold to a funnel shift (see rotate.ll) - not sure whether its worth keeping or not.

spatel accepted this revision.Nov 2 2020, 1:35 PM

LGTM

This revision is now accepted and ready to land.Nov 2 2020, 1:35 PM
This revision was landed with ongoing or failed builds.Nov 3 2020, 3:03 AM
This revision was automatically updated to reflect the committed changes.
spatel reopened this revision.Nov 4 2020, 5:28 AM

Reopening because the patch was reverted.
Failure example from the reverting commit:

$ cat apedec-reduced.c
a;
b(e) {
  int c;
  unsigned d = f();
  c = d >> 32 - e;
  return c;
}
g() {
  int h = i();
  if (a)
    h = h << a | b(a);
  return h;
}
$ clang -target aarch64-linux-gnu -w -c -O3 apedec-reduced.c
clang: ../lib/Transforms/InstCombine/InstructionCombining.cpp:3656: bool llvm::InstCombinerImpl::run(): Assertion `DT.dominates(BB, UserParent) && "Dominance relation broken?"' failed.
This revision is now accepted and ready to land.Nov 4 2020, 5:28 AM
RKSimon updated this revision to Diff 312849.Dec 18 2020, 10:36 AM

Ensure we block poison in a funnel shift value - similar to rG0fe91ad463fea9d08cbcd640a62aa9ca2d8d05e0

RKSimon planned changes to this revision.Dec 18 2020, 10:36 AM

I still haven't been able to get to the bottom of PR48068

RKSimon updated this revision to Diff 312966.Dec 20 2020, 3:30 AM
RKSimon added a reviewer: mstorsjo.

Check that the shift values can be hoisted from each basic block.

This revision is now accepted and ready to land.Dec 20 2020, 3:30 AM
RKSimon requested review of this revision.Dec 20 2020, 3:30 AM

The original, unreduced testcase seems to work fine now at least - thanks! Can't really comment on the code itself though.

spatel accepted this revision.Dec 21 2020, 6:19 AM

LGTM - the only diff from the earlier rev is that we now check the dominator tree to confirm that the shift ops are safe to hoist.

This revision is now accepted and ready to land.Dec 21 2020, 6:19 AM