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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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);
----------------
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.


================
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(
----------------
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.


================
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(
----------------
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.


================
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]]
----------------
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.


================
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:
> 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
}

```


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

https://reviews.llvm.org/D109807