[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:1646
+  // Reassociate chains of Ops {and,or,xor} where one side is an extend.
+  // extend(X) | (extend(Y) | Z) --> (extend(X) | extend(Y)) | Z
+  // (extend(X) | extend(Y)) may then be further optimized to narrow the
----------------
This code comment is not accurate - we allow non-extend casts for one of the operands. See comment on the tests below.


================
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]]
----------------
mnadeem wrote:
> 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?
Sure, that is one example of different cast/type. 

But there's an inconsistency here. I don't think you intended it, and I don't think we want it: why are we reassociating any cast as long as the source type is an int or int vector, but not allowing FP casts/types? 

It's fine if this patch only deals with pairs of extends or it allows any cast ops, but we're in some in-between state currently if I'm seeing it correctly. We want to avoid an arbitrary line in canonicalization logic.

Please add these tests:

```
define i32 @zext_bitcast_int_vec_and_chain(i32 %a, i16 %b, <2 x i16> %c) {
  %conv = zext i16 %b to i32
  %conv2 = bitcast <2 x i16> %c to i32
  %and = and i32 %conv, %a
  %and2 = and i32 %and, %conv2
  ret i32 %and2
}

define i32 @zext_bitcast_fp_vec_and_chain(i32 %a, i16 %b, <2 x half> %c) {
  %conv = zext i16 %b to i32
  %conv2 = bitcast <2 x half> %c to i32
  %and = and i32 %conv, %a
  %and2 = and i32 %and, %conv2
  ret i32 %and2
}

```


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

https://reviews.llvm.org/D109807