[PATCH] D109807: [InstCombine] Narrow type of logical operation chains in certain cases

[Usman Nadeem via Phabricator via llvm-commits]

mnadeem added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1613
   CastInst *Cast0 = dyn_cast<CastInst>(Op0);
-  if (!Cast0)
-    return nullptr;
+  if (!Cast0) {
+    std::swap(Op0, Op1);
----------------
spatel wrote:
> mnadeem wrote:
> > spatel wrote:
> > > It's not clear to me why this swap is necessary. Do you have a test case where a logic binop has a cast as operand 0 and a binop as operand 1? Complexity-based canonicalization is supposed to prevent that. See InstCombinerImpl::SimplifyAssociativeOrCommutative().
> > I think you have it the other way around, the code currently only checks for the cast to be on the LHS.
> > This swap handles the case when the Op0 (higher complexity) is another binop.
> Ah, I see. 
> 
> I wonder if it would be easier to read if we just match this in its "natural" form then:
> (extend(X) | Y) | extend(Z) --> (extend(X) | extend(Z)) | Y
> 
> You would hoist this code above the current bailout for !Cast0 if you did it that way.
Relying on the exit below to keep things simple:

```
  if (!SrcTy->isIntOrIntVectorTy())
    return nullptr;
```

I can hoist the code but then I would have to add tests for floating point conversions.


================
Comment at: llvm/test/Transforms/InstCombine/and-xor-or.ll:511
 
 define i64 @sext_or_chain_two_uses2(i64 %a, i16 %b, i16 %c, i64 %d) {
 ; CHECK-LABEL: @sext_or_chain_two_uses2(
----------------
spatel wrote:
> This test doesn't add much value. In InstCombine, I don't think we ever care if the instruction that is being replaced has multiple uses.
Removed the test


================
Comment at: llvm/test/Transforms/InstCombine/and-xor-or.ll:551
 
 define i64 @sext_or_chain_two_uses4(i64 %a, i16 %b, i16 %c, i64 %d) {
 ; CHECK-LABEL: @sext_or_chain_two_uses4(
----------------
spatel wrote:
> Similar to test comment above: this does not add value vs. the previous test if it is just an extra use of the final value.
Removed the test


================
Comment at: llvm/test/Transforms/InstCombine/and-xor-or.ll:591-592
 ; CHECK-NEXT:    [[CONV2:%.*]] = sext i16 [[C:%.*]] to i64
-; CHECK-NEXT:    [[XOR:%.*]] = xor i64 [[CONV]], [[A:%.*]]
-; CHECK-NEXT:    [[XOR2:%.*]] = xor i64 [[XOR]], [[CONV2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = xor i64 [[CONV2]], [[CONV]]
+; CHECK-NEXT:    [[XOR2:%.*]] = xor i64 [[TMP1]], [[A:%.*]]
 ; CHECK-NEXT:    ret i64 [[XOR2]]
----------------
spatel wrote:
> That seems like a reasonable canonicalization (and the 'and' test below shows a potential win)...
> But we should note that this is intentional in a code comment.
> 
> But what happens if "%conv" is `trunc` or `fptoui` or some other non-ext cast? 
> 
> Please add test(s) with those patterns.
Added the comments in one of the other tests above and also in the c code.


> But what happens if "%conv" is trunc or fptoui or some other non-ext cast?

added `zext_trunc_and_chain` etc at the bottom. Is this what you meant?


================
Comment at: llvm/test/Transforms/InstCombine/and-xor-or.ll:474
 
 ; Negative test with more uses.
 define i64 @sext_or_chain_two_uses1(i64 %a, i16 %b, i16 %c, i64 %d) {
----------------
spatel wrote:
> spatel wrote:
> > I think we're missing tests:
> > 1. Negative test with different logic opcodes.
> > 2. Negative test with different cast opcodes.
> > 3. Test with different cast source types.
> > 4. Test with multiple uses of cast instruction(s).
> > 5. Tests where the first cast is operand 1 of the logic op (notice in the original tests that the operands are commuted from where they started - search around the test directory for "thwart complexity-based canonicalization" for ways to prevent that).
> I don't see an example of "3" here yet. I'm imagining something like this:
> 
> 
> ```
> define i64 @f(i64 %a, i8 %b, i16 %c) {
>   %conv = zext i8 %b to i64
>   %conv2 = sext i16 %c to i64
>   %xor = xor i64 %conv, %a
>   %xor2 = xor i64 %xor, %conv2
>   ret i64 %xor2
> }
> 
> ```
added `zext_xor_chain_diffSrcTy` above it is similar to your snippet but with both zext casts. 


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

https://reviews.llvm.org/D109807