[PATCH] D55604: [AggressiveInstCombine] convert rotate with guard branch into funnel shift (PR34924)

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 3 inline comments as done.
spatel added inline comments.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:95
+    return Intrinsic::not_intrinsic;
+  };
+
----------------
nikic wrote:
> Rather than matching the rotate pattern here, would it be possible to leave the conversion to fsh to InstCombine (as you say, this is planned anyway), and only do the removal of the zero check here? Or is general canonicalization in InstCombine still blocked on some optimization issues?
There's no clear limit on the power of aggressive-instcombine, but I'm assuming that we should follow the same rules as regular instcombine. Ie, you should be able to transfer anything in here to instcombine if you don't care about compile-time.

So we defer CFG changes to other passes (simplifycfg in particular) and just focus on logical transforms. This one blurs the lines of course, but I don't think we should be trying to alter the branch rather than the phi here.

The only remaining optimization issue that I'm aware of for funnel shift canonicalization is addressed by D55563.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:110
+           "Pattern must match funnel shift left or right");
+  }
+
----------------
nikic wrote:
> Do we possibly need to check that the rotate only has a single use, to avoid converting both the phi into fsh and leaving behind the original rotate?
Sure, I can add that check. Strictly speaking, we can never create more instructions than we started with here (we're only replacing the phi), so we don't need to do that.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:124
+
+  if (Pred != CmpInst::ICMP_EQ || TrueBB != Phi.getParent() || FalseBB != RotBB)
+    return false;
----------------
nikic wrote:
> Is `icmp ne %x, 0` already canonicalized to `icmp eq %x, 0` beforehand?
Yes, instcombine prefers 'eq' to 'ne', so we're relying on that. It's possible that we don't get that transform though - fresh example of that:
https://bugs.llvm.org/show_bug.cgi?id=39968
...so like the 'sle' negative test that's already included here, we are not catching every possible pattern in this patch. But until there's evidence that we need it, I don't think it's worth chasing. 


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

https://reviews.llvm.org/D55604