[PATCH] D108091: [AggressiveInstCombine] Add shift left instruction to `TruncInstCombine` DAG

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.

LGTM too - see inline for test suggestions.



================
Comment at: llvm/test/Transforms/AggressiveInstCombine/trunc_shifts.ll:4
 
 define i16 @shl_1_commute(i8 %x) {
 ; CHECK-LABEL: @shl_1_commute(
----------------
lebedev.ri wrote:
> anton-afanasyev wrote:
> > lebedev.ri wrote:
> > > Pedantic nitpick: commute means `mul %x, %y   <=>   mul %y, %x`
> > > Here you want to use `negative_test` in place of `not_commute`, and drop `commute`.
> > "Commute" here means that `trunc` and `shl` commutes as operators: `trunc ∘ shl = shl ∘ trunc`, i.e. `trunc(shl(*, *)) = shl(trunc(*), trunc(*)))`. But I'm to change it, thanks.
> I have literally never seen such usage in all of LLVM. But that may be sample size issue.
> Usually it's marked as `fold`.
I was also confused by the use of "commute" here. The file name says we're truncating shifts, so I'd just make these all "shl..." or "narrow_shl..." (assuming we'll add the other shift ops in follow-up patches).


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/trunc_shifts.ll:36-37
   %zext = zext i8 %x to i32
   %shl = shl i32 %zext, 16
   %trunc = trunc i32 %shl to i16
   ret i16 %trunc
----------------
anton-afanasyev wrote:
> spatel wrote:
> > If the left shift amount >= the truncated bit-width, the result must be 0.
> > Looks like -instcombine gets this, but not -instsimplify. 
> > Can we adjust this test to something that does not completely fold away?
> Hmm, yes, you're right. But this could be adjusted only if shift amount is variable, this case is covered by `@shl_var_not_commute()` function below. I can only remove this test at all.
It's fine to leave here. It does suggest a possible optimization (or two) - we could be trying to call SimplifyInst or similar utility to make sure the IR is reduced coming in or going out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108091