[PATCH] D62818: [InstCombine] Introduce fold for icmp pred (and X, (sh signbit, Y)), 0.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/onehot_merge.ll:18-23
+;CHECK:       [[TMP:%.*]] = shl i32 1, %c1
+;CHECK-NEXT:  [[TMP1:%.*]] = and i32 [[TMP]], %k
+;CHECK-NEXT:  icmp eq i32 [[TMP1]], 0
+;CHECK-NEXT:  [[TMP2:%.*]] = shl i32 %k, %c2
+;CHECK-NEXT:  icmp sgt i32 [[TMP2]], -1
+;CHECK-NEXT:  or i1
----------------
spatel wrote:
> lebedev.ri wrote:
> > huihuiz wrote:
> > > spatel wrote:
> > > > lebedev.ri wrote:
> > > > > lebedev.ri wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > Can you please regenerate the original test?
> > > > > > I'm not sure what's on the LHS of the diff, but ignoring the instruction count this looks like improvement to me.
> > > > > AH, you also want to str-replace `%tmp` with `%t`, it confuses the update script likely.
> > > > rL364546
> > > Actually we missed the fold for 
> > > 
> > > ```
> > > ((k & ( 1 l<< C1 )) == 0) || ((k & ( signbit l>> C2 )) == 0)
> > > -->
> > > ((k & (( 1 l<< C1 ) || ( signbit l>> C2 ))) != 0)
> > > ```
> > Thanks for the analysis!
> > @spatel does this fall into the nowadays reasoning that we shouldn't be doing too much folds into bitmath
> > here in instcombine? I'm almost tempted to say that this isn't a regression,
> > but the original fold that now no longer happens should be removed instead.
> > 
> If we say that the longer IR sequence is more canonical, then we'd want to add a transform to create that longer sequence starting from the shorter sequence. Are we willing to do that to improve analysis in IR?
> 
> As a practical matter, we probably also want to look at asm output for the alternatives on a few targets to see how much backend logic is required to do/undo this.
Sorry - I haven't followed this patch and its friends closely; scrolling back through the comments, I think the backend questions are covered by D62871.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62818