[PATCH] D65530: [InstCombine] foldXorOfICmps(): don't give up on non-single-use ICmp's if all users are freely invertible

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D65530#1625331 <https://reviews.llvm.org/D65530#1625331>, @lebedev.ri wrote:

> In D65530#1625321 <https://reviews.llvm.org/D65530#1625321>, @spatel wrote:
>
> > Make sure I didn't typo this translation, but I think I'm seeing extra instructions for vectors with this transform on x86 and aarch64:
> >  ...
>
>
> Sure, but that is irrelevant. D65765 <https://reviews.llvm.org/D65765> improves upon both of them:
>  https://godbolt.org/z/-UIddL


Ok, but I wouldn't call it irrelevant if this patch is viewed/committed independently. For example, if D65765 <https://reviews.llvm.org/D65765> had to be reverted, we'd probably want to revert this too, so there's no perf regression potential.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2654
 
-Value *InstCombiner::foldXorOfICmps(ICmpInst *LHS, ICmpInst *RHS) {
+// Given i1 V, can every user of V can be freely adapted if V is changed to !V ?
+static bool CanFreelyInvertAllUsersOf(Value *V, Value *IgnoredUser) {
----------------
spatel wrote:
> Is there a way to share logic with the existing IsFreeToInvert()? Can we put them next to each other in 1 file, so it's easier to discover that both exist?
'V can be' -> 'V be'


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2654-2655
 
-Value *InstCombiner::foldXorOfICmps(ICmpInst *LHS, ICmpInst *RHS) {
+// Given i1 V, can every user of V can be freely adapted if V is changed to !V ?
+static bool CanFreelyInvertAllUsersOf(Value *V, Value *IgnoredUser) {
+  // Look at every user of V.
----------------
Is there a way to share logic with the existing IsFreeToInvert()? Can we put them next to each other in 1 file, so it's easier to discover that both exist?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2749
+      if (X && Y && (Y->hasOneUse() || CanFreelyInvertAllUsersOf(Y, &I))) {
+        // Invert the predicate of 'Y', thus inverting it's output.
+        Y->setPredicate(Y->getInversePredicate());
----------------
it's -> its


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2756
+          // immediate instruction count, we have just ensured that all the
+          // users are freely-inversible, so that 'not' *will* get folded away.
+          BuilderTy::InsertPointGuard Guard(Builder);
----------------
inversible -> invertible


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65530