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

Thu Dec 8 05:35:16 PST 2022

spatel added a comment.

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138814