[PATCH] D114666: [InstSimplify] Simplify bool icmp with not in LHS

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I pushed the baseline tests with:
64d4bd02dc3f <https://reviews.llvm.org/rG64d4bd02dc3f8741043806c82a87f81aff7bc8d9>

So this patch can be rebased/updated to show diffs on those (and we can confirm that the transform is correct by comparing against the instcombine results).



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2708
   // A boolean compared to true/false can be simplified in 14 out of the 20
-  // (10 predicates * 2 constants) possible combinations. Cases not handled here
-  // require a 'not' of the LHS, so those must be transformed in InstCombine.
+  // (10 predicates * 2 constants) possible combinations.
+
----------------
The code comment should not be deleted. It is referring to patterns like this:
https://alive2.llvm.org/ce/z/3rhGa5
In that case, we can't do anything in instsimplify because we do not create new values here. But instcombine has the ability to create a new instruction.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2712-2713
+  // E.g not(X) == 0 -> X != 0
+  Value *LHS_Not = LHS;
+  CmpInst::Predicate PredNot = Pred;
+
----------------
Please make a helper function or lambda to give these values smaller scope. It is not accurate to name these "*Not" in the case that we are not matching a 'not' instruction, so a different (more generic) name would be better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114666