[PATCH] D140858: [InstCombine]: Don't simplify bits if it causes imm32 to become imm64

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D140858#4023792 <https://reviews.llvm.org/D140858#4023792>, @goldstein.w.n wrote:

> It works, what I expect is happening is by the time code is in the DAG the `llvm.assume` has been removed.
> Unless we want to propegate `llvm.assume` to the backends there seems to be a genuine
> reason to keep this in InstCombine.

Yes, that's correct, llvm.assume is not preserved in SDAG. If the motivation here are assumes in particular, then we cannot undo in the backend.

In D140858#4023805 <https://reviews.llvm.org/D140858#4023805>, @goldstein.w.n wrote:

> In D140858#4022695 <https://reviews.llvm.org/D140858#4022695>, @nikic wrote:
>
>> Of course, doing this in InstCombine may also make sense -- the test diffs look like this might be slightly beneficial at the IR layer for the pattern emitted by LoopVectorize (e.g. see diffs in LoopVectorize/X86/small-size.ll). If we do want this in InstCombine, I'd expect a more principled approach that does not hardcode specific sizes. E.g. we could generally avoid masking off any sign bits, to avoid increasing the number of significant (signed) bits.
>
> Can you expand on the rationale behind not using `TTI.getIntImmCostInst` here? This doesn't seem like a target specific transform and "is this imm better than that one?" seems like an inherently target specific question.

InstCombine is a target-independent canonicalization pass. It produces IR in a canonical form that other passes (or other transforms in InstCombine for that matter) can rely on. This canonical IR is independent of the target, beyond an unavoidable dependence on the DataLayout.

We //could// loosen this restriction (this would require an RFC on discourse), but at this point I don't really see evidence that it would be necessary to handle this case.

If we avoid shrinking that increases significant bits, the regressions I see are in `@scalar_i32_lshr_and_negC_eq_X_is_constant1` and `@scalar_i32_lshr_and_negC_eq_X_is_constant2`, so we'd want to think about whether these can be avoided. (This would be necessary regardless of restrictions on the transform, it's just essentially impossible to find these cases if target-dependence is involved.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140858