[PATCH] D127903: [InstCombine] Optimize test for same-sign of values

[Eric Gullufsen via Phabricator via llvm-commits]

emgullufsen added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2640
+      InstCombiner::isSignBitCheck(PredR, *RHSC, TrueIfSignedR) &&
+      RHS->hasOneUse() && LHS->hasOneUse()) {
+    Value *X, *Y;
----------------
spatel wrote:
> The use checks are too strict. This optimization should be a win as long as any one of the intermediate values has one use. We are creating 2 new values, so as long as we can remove one of the old ones + the final value, it is an improvement.
> 
> I'd make this:
>   (LHS->hasOneUse() || RHS->hasOneUse())
> ..and remove the use checks for LHS0 and RHS0 under here.
Alrighty I see now - sounds good thank you - will amend in updated diff.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2654-2658
+      Value *NewC = IsAnd ? ConstantInt::getNullValue(NewXor->getType())
+                          : ConstantInt::getAllOnesValue(NewXor->getType());
+      ICmpInst::Predicate NewPred =
+          IsAnd ? ICmpInst::ICMP_SLT : ICmpInst::ICMP_SGT;
+      return Builder.CreateICmp(NewPred, NewXor, NewC);
----------------
spatel wrote:
> There's a Builder shortcut to create the new ICmp: `CreateIsNeg` / `CreateIsNotNeg`.
Aha very cool thank you - will amend in updated diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127903