Are you able to add any tests supporting this?

In D154533#4480255, @barannikov88 wrote:

In this case, we replace a complex web of instructions by just one add. Reconstructing a carry is worth it. The test cases that were changed bellow are good exemple of that, the carry is passed as an argument to the functions, so it is not possible to know if these are "real" carries or not, and it is nevertheless worth doing the transform.

I was thinking about cases like feeding uaddo by a setcc, but that does not seem to pay off. E.g.:

declare { i64, i1 } @llvm.usub.with.overflow.i64(i64, i64)

define { i64, i1 } @subcarry_fake_carry(i64 %a, i64 %b, i32 %x, i32 %y) {
    %t1 = call { i64, i1 } @llvm.usub.with.overflow.i64(i64 %a, i64 %b)
    %partial = extractvalue { i64, i1 } %t1, 0
    %k1 = extractvalue { i64, i1 } %t1, 1

    %carryin = icmp eq i32 %x, %y
    %zcarryin = zext i1 %carryin to i64
    %s = call { i64, i1 } @llvm.usub.with.overflow.i64(i64 %partial, i64 %zcarryin)
    %k2 = extractvalue { i64, i1 } %s, 1

    %carryout = or i1 %k1, %k2

    %ret = insertvalue { i64, i1 } %s, i1 %carryout, 1
    ret { i64, i1 } %ret
}

Removed TODO comments from tests.

Update tests with --check-globals

Rebased on new tests cases.

Focus on select+binop, add more test cases.

In D153453#4500378, @serge-sans-paille wrote:

Could you also add the test case from https://reviews.llvm.org/D155207 ? It was reproducing the issue I had when compiling firefox.

Thanks for working on this and good luck.

In D138814#3953742, @spatel wrote:

In D138814#3953729, @foad wrote:

What do you think about producing the sequence with 'not' + 'icmp' instead of the uadd + extract?

The point about using uadd.with.overflow is that it also gives you the truncated 32 bit result of the add.

If we're not using that value (as shown in the first two tests), then do we still we want to canonicalize to an overflow intrinsic? That implies that we also need to transform the "not+icmp" pattern into "uadd+extract" to be consistent.

Rebased.

In D56214#3885105, @RKSimon wrote:

@chfast What do you want to do with this patch now that D136015 landed?

LGTM, but I'd wait for one more review.

In D136015#3876878, @RKSimon wrote:

In D136015#3875187, @chfast wrote:

In https://reviews.llvm.org/D56214 similar pattern match was applied in AggressiveInstCombine.

Do you want me to submit test cases from there?

Yes please @chfast, if you think we can just use this patch then maybe just move them (and tweak for -instcombine).

In https://reviews.llvm.org/D56214 similar pattern match was applied in AggressiveInstCombine.

Is it fine I commit this?

Rebased.

I added tests for this change.
https://reviews.llvm.org/rGe399c5877801
https://github.com/llvm/llvm-project/commit/e399c58778017388a818cabd1b13fcb88b79a432

In D127115#3803060, @RKSimon wrote:

@deadalnix - reverse ping

In D118037#3740763, @deadalnix wrote:

In D118037#3731784, @RKSimon wrote:

Please can you rebase to see where we currently are? Also D127115 affects these tests - so maybe see whether this SETULT change is still needed after that patch or whether something else can further improve the codegen?

For what it is worth, when the dag is processed in fully topological order, and you add in D57317 , then you get optimal codegen for these tests.

Rebased.

Should I try to push this by adding a TTI flag like "CmpULTsetsCarryFlag"?

Looks very nice, thanks.

In D56214#3377947, @RKSimon wrote:

@chfast What happened to this patch? I was looking at https://llvm.org/PR36243 and wondering if it'd be worth AggressiveInstCombine/InstCombine doing something similar for adds (in that case a 3 chain add i32 to add i96).

On the high level note, using builtin_subc() or builtin_sub_with_overflow() is good enough workaround.

In D118037#3377941, @RKSimon wrote:

@chfast reverse-ping - any luck with working out how best to deal with add_U320_without_i128_add?

Rebased.

This was intermediate change to solve https://github.com/llvm/llvm-project/issues/53432. There I want to merge sub + cmp into single sbb. Any suggestions how to solve it differently?

In D118037#3294363, @RKSimon wrote:

Maybe perform this inside X86ISelLowering?

What I try to do in the end is to fix https://github.com/llvm/llvm-project/issues/53432. Maybe this particular change is not good direction, but on the other hand it also helps with the test add_U320_without_i128_add already in repo.

In D118037#3272270, @RKSimon wrote:

BTW - how come we still have the ISD::ADDE/SUBE nodes? I thought we were intending to get rid of them as the glue nodes made them difficult to work with.

In D118037#3272185, @lebedev.ri wrote:

Is ISD::SETULT documented as affecting carry flag?

Use switch. Rebased.

Address review comments.

What do you think about this idea in general? Can we assume or check if SETULT actually affects CL flag on given target?

I have noticed this post about undef/poison: https://llvm.discourse.group/t/evolution-of-undef-and-poison-over-time/5917/2.
Should I have added tests with poison instead of undef?

Trigger rebuild.