[PATCH] D138814: [InstCombine] Combine a/lshr of add -> uadd.with.overflow

Thu Dec 8 05:36:33 PST 2022

arsenm added a comment.

In D138814#3981276 <https://reviews.llvm.org/D138814#3981276>, @spatel wrote:

> In D138814#3980701 <https://reviews.llvm.org/D138814#3980701>, @Pierre-vh wrote:
>
>> In D138814#3973599 <https://reviews.llvm.org/D138814#3973599>, @nikic wrote:
>>
>>> FWIW our historical stance has always been that uadd.with.overflow is non-canonical, and the canonical pattern is `a + b < a` (for non-constant `b`). uadd.with.overflow generally has worse optimization support, which is why we only form it during CGP for backend purposes.
>>
>> Interesting, not sure what other reviewers think?
>> Maybe adding a TII hook so targets can enable/disable the combine is a good idea? e.g. something like `allowUAddoCanonicalForm`?
>
> We don't want TTI/TLI hooks in InstCombine because it's supposed to be early/target-independent transforms only (although it has folds gated by the data-layout that are almost the same thing as using TTI).
>
> We decided that there is a legitimate need for target-dependent canonicalization though, so that's now possible in AggressiveInstCombine. So moving this patch to that pass and gating the transform on a target hook seems like a non-controversial way forward.
>
> Currently, CodeGenPrepare uses this hook:
> https://github.com/llvm/llvm-project/blob/1fe65d866c5285261b7766f2d3930ae975b878ff/llvm/include/llvm/CodeGen/TargetLowering.h#L3086
>
> If we're using that as an early predicate, then I think it should be moved to TargetTransformInfo. I don't see any uses outside of CGP currently.

I'm more inclined to do this in CGP than AggressiveInstCombine if the shift is the preferred canonical form

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138814