[Openmp-commits] [PATCH] D125717: [InstCombine] Optimize and of icmps with power-of-2 and contiguous masks

[John McIver via Phabricator via Openmp-commits]

jmciver added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:610
 /// Try to fold (icmp(A & B) ==/!= C) &/| (icmp(A & D) ==/!= E)
-/// into a single (icmp(A & X) ==/!= Y).
+/// into a single (icmp(A & X) ==/!= Y) or (icmp A u< Y).
 static Value *foldLogOpOfMaskedICmps(ICmpInst *LHS, ICmpInst *RHS, bool IsAnd,
----------------
goldstein.w.n wrote:
> When do we transform to `A & X eq/ne Y`? I don't see that code.
That is a great question. The `A & X eq/ne Y` is emitted by the other transformations in `foldLogOpOfMaskedICmps`. When I started developing this patch I picked this location as the classifier function `getMaskedTypeForICmpPair` converted the source to the form:
`(icmp ne (A & B), C) | (icmp qe (A & D), E)`. This is violation of the original intended use `foldLogOpOfMaskedICmps` at it is targetting ICMPs of the same operator, which fold to `(icmp(A & X) ==/!= Y)`.

My current thinking is to move this fold into one of the other called functions in `foldAndOrOfICmps` (option 1) or create its own function and then call that from `foldAndOrOfICmps` (option 2).  

Option 1: One possible function is `foldIsPowerOf2`, but it is specific to `ctpop` based expressions so I am hesitant to expand functionality.

Option 2: Create a function called `foldIsNegativePowerOf2` (open to other names), which will attempt the transform `X <u 2^n` to `(X & ~(2^n-1)) == 0` and then perform the subsequent tests for folding as seen in `foldLogOpOfMaskedICmpsAllZerosBMaskNeqMixedOrContiguous`. This removes the need for verbose aforementioned function name and provides a concise "does one thing well" function.

I think option 2 is probably the best path. I'll go ahead and implement and see what it looks like. If you have other ideas let me know!


================
Comment at: llvm/test/Transforms/InstCombine/icmp-logical.ll:2553
   ret <6 x i1> %t4
 }
----------------
goldstein.w.n wrote:
> One more thing. Can you put the "fail" keyword somewhere in the tests that you expect no change in. Think it helps make it crystal clear exact what the is meant to happen.
> 
I updated the names of tests that intentionally result in no change with a "_fail" postfix, see D143044.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125717