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

[Noah Goldstein via Phabricator via Openmp-commits]

goldstein.w.n 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,
----------------
jmciver wrote:
> 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!
Bah, I had left reply unsent. Agree option 2 is better (and looks like thats what you did so great)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125717