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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D140858#4025563 <https://reviews.llvm.org/D140858#4025563>, @nikic wrote:

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

Fair enough, will drop it.

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

Fixed those two cases. The issue was there was an assumption that `Known.Zero & Value == 0` so it was using `~Known.Zero + Value == ~Known.Zero | Value`. It would have broken on the original change had it tested with `i64` in addition to `i32`

There is one more case in `llvm/test/Transforms/LoopVectorize/induction.ll` where we regress from `zext` -> `sext`.
I took a bit of a loop, the issue is essentially by unsetting bits, we are essentially propegating analysis information. In `InstCombinerImpl::visitSExt` the check to `isKnownNonNegative(Src, DL, 0, &AC, &CI, &DT)` never finds the prior comparison (that was available `InstCombineCompares`) and since the `and` itself doesn't prohibit the sign but, it evaluates to false.

My feeling is we have 2 choices:

1. Allow removal of sign bit if it transforms `imm8 -> imm16/imm32` and `imm16 -> imm32` as those all the types where we can get the `sext` -> `zext` optimization.
2. Allow arbitrary bit simplification in `InstCombine` and undo it in a later pass (where maybe we can use TTI) and we still have llvm.assume.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140858