[PATCH] D74228: [PatternMatch] Match XOR variant of unsigned-add overflow check.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D74228#1870505 <https://reviews.llvm.org/D74228#1870505>, @spatel wrote:

> In D74228#1870169 <https://reviews.llvm.org/D74228#1870169>, @fhahn wrote:
>
> > In D74228#1869653 <https://reviews.llvm.org/D74228#1869653>, @spatel wrote:
> >
> > >
> >
> >
> > The example below is interesting! It seems like we have the same problem with the add version of UAddWithOverflow, as we also generate uadd calls if the sum is not used besides the compare (e.g. the @uaddo1 test case in llvm/test/Transforms/CodeGenPrepare/X86/overflow-intrinsics.ll)
> >
> > Maybe we should adjust the shouldFormOverflowOp hook to check if there are actual users of the sum and default to OFF if there are actual users? And then opt in on X86 & AArch64, where I think it should be beneficial for overflow-only checks. I think doing it in CGP is more convenient then in SelDag and we would also need an appropriate target hook there I think. Unless we match that pattern in the target implementations.
> >
> > What do you think?
>
>
> I think it would be fine to tighten up the default hook and then allow targets to opt in with more awareness. OTOH in SDAG, we could just check that UADDO is legal/custom, so no custom hook needed?


So should I prepare a set of patches for the improved target hook? Or SelDag? I would slightly prefer CGP for that, also with GlobalISel in mind and that's were we already do the expansion for similar cases.

> 
> 
>> I think that should cover it, thanks for sharing the problematic case on SPARC.
> 
> That took some hunting. :)
>  But I just wanted to show that we need to be careful with these transforms. We've generated a lot of back-and-forth regressions with the overflow intrinsics.

+1, I've run into a few I am trying to address with this and other patches in the area :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74228