[PATCH] D43835: Simplify more cases of logical ops of masked icmps.

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi marked an inline comment as done.
yamauchi added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:454
+  // If IsAnd is false, we get it in negated form:
+  //   (icmp eq (A & B), 0) | (icmp ne (A & D), E) ->
+  //      !((icmp ne (A & B), 0) & (icmp eq (A & D), E)).
----------------
davidxl wrote:
> replace 0 with 'C' in the example.
LHS being of type Mask_NotAllZeroes means that LHS is or can be converted into the form "(icmp ne (A & B), 0)". I think 0 would be better here because the right hand side of the LHS isn't necessarily C. Note the case of Mask_NotAllZeroes where B==C and B is a power of two (see lines 216 and 269 above) where we get "(icmp eq (A & B), C)" (B==C) but interpret it as (equivalent) "(icmp ne (A & B), 0)".


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:462
+    return nullptr;
+  ConstantInt *CCst = dyn_cast<ConstantInt>(C);
+  if (!CCst)
----------------
davidxl wrote:
> Do you need to check CCst is Zero?
Similarly, C isn't necessarily zero if Mask_NotAllZeroes is due to the case for B==C and B is a power of two. So, no.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:474
+
+  // Update E to the canonical form when D is a power of two and RHS is
+  // canonicalized as,
----------------
davidxl wrote:
> It seems to me, it is sufficient to do the following check:
> 
> 1) check if mask B is a subset of mask D
> 2) check if (E & B) != 0 is true 
> 
> If both 1) and 2) are satisfied, the simplification is legal.
I need a clarification on this.

I think the case you are referring to, which is an important one, is covered by lines 552-553 below. But there are several other cases of simplifications below. There are simplifications we can do even when B isn't a subset of D below, eg. (icmp ne (A & 255), 0) & (icmp eq (A & 15), 8) -> (icmp eq (A & 15), 8).

Do you mean we can drop the other cases?


Repository:
  rL LLVM

https://reviews.llvm.org/D43835