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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:95
+    return Intrinsic::not_intrinsic;
+  };
+
----------------
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?


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:110
+           "Pattern must match funnel shift left or right");
+  }
+
----------------
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?


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:124
+
+  if (Pred != CmpInst::ICMP_EQ || TrueBB != Phi.getParent() || FalseBB != RotBB)
+    return false;
----------------
Is `icmp ne %x, 0` already canonicalized to `icmp eq %x, 0` beforehand?


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

https://reviews.llvm.org/D55604