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

Mon Feb 10 07:11:59 PST 2020

fhahn added a comment.

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

> In D74228#1865537 <https://reviews.llvm.org/D74228#1865537>, @nikic wrote:
>
> > Not sure this is the right way to go about it. As we're only using the "overflow" result here, and don't need to collect two nominally independent instructions (potentially from different BBs), I don't think there's a strong reason to do this transform in CGP, as opposed to a setcc->uaddo DAG combine (if uaddo is legal).
> >
> > Maybe @spatel can chime in, who implemented the handling in CGP.
>
>
> IIRC, the history of transforms for overflow ops is something like this:
>
> 1. We tried to canonicalize to them in instcombine, but this caused perf regressions.
> 2. We converted some of them in CGP instead.
> 3. We converted more of them in CGP (this was my attempt), but this caused perf and compile-time regressions.
> 4. We crippled the CGP transforms to avoid regressions.
>
>   I'm not sure what the status is currently in SDAG. Given that we already have the code in CGP, and this is a small addition, I wouldn't object to extending that code. If we could re-implement everything that is in CGP in SDAG, that might be a better option...although that might be counter-productive if we assume we're soon heading to or are already in a GlobalISel world.
>
>   I might have missed some of the recent patches on overflow intrinsics. It's not clear to me from the diff what InstCombine's role in this patch is - can we add a code comment and/or regression test to explain what we expect to happen for this (and other patterns)?

InstCombine started turning (a + b <u a)  into (a ^ -1 <u b), but CGP looks for  (a + b <u a)  and misses the case now. Does that answer your question?

It might be good to have tests that run instcombine & CGP to catch regressions such as this?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74228