Thanks for the review @efriedma

Ping?

Oops, I forgot to add the Differential URL in the commit.

Thanks for the review @asb !

Thanks @efriedma looks good to me.

Hi Tom,

There is a flag -fno-ident that has the same effect in GCC https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-ident although it involves also ignoring the #ident.

Looks good to me. Thanks!

LGTM now. Thanks!

Thanks a lot Marten.

Looks good to me now. Wait a couple of days before submitting it just in case the other reviewers have more comments.

Let's see how this one goes :)

ChangeLog:

• Rebase with ToT

Can you coalesce them in a single regular expression, like /;\s*CHECK-(NEXT|NOT|DAG|SAME|LABEL): (not sure if this is the right syntax but I hope you got the idea.

This LGTM. Thanks for fixing this.

Ping :-)

I understand you're fixing the narrowing check from "unscoped enum/integer type" → float.

@efriedma @samparker any further comments on this change? We can add more clz-like tricks in later changes if needed.

Thank you very much @efriedma !

Ping :-)

ChangeLog:

• Remove whitespace and unused variables in LowerADDSUBCARRY

ChangeLog:

• Change ConvertBooleanCarryToCarryFlag to use ARMISD::SUBC x, 1 instead of ARMISD::ADDC x, ~0, this way a single combiner in PerformAddcSubcCombine is enough.
• Do not use ISD::SUBCARRY if the third operand is a zero. While the generic combiner can lower this to ISD::USUBO it may not have a chance to run before we lower that ISD::SUBCARRY.

Update patch based on the review comments.

Hi @aemerson thanks a lot for the prompt fix.

Hi @efriedma gluing the nodes is too disruptive so I am exploring one of the alternatives you suggested. What do you think, does this look workable?

Hi @efriedma thanks a lot for the suggestions and the testcase. I already considered gluing though it impacts scheduling and the change will be noisy for tests but I'll look into your other suggestions too.

Ping. Modulo the bugs we may find with ADDCARRY / SUBCARRY further thoughts on this?

LGTM. Wait a couple of days before commiting just in case @efriedma has further comments.

LGTM. Please wait a couple of days before submitting just in case @efriedma has further comments.

Thanks @efriedma !

ChangeLog

• New tests in D41122 are now updated in this change to show the change in codegen

ChangeLog:

• New tests before D34515 is applied showing current codegen (and not after like I did the last time)

Oh, that makes a lot of sense. I'll do. Sorry for the misunderstanding.

ChangeLog:

• I forgot to update the Thumb counterpart of long-setcc.ll in the last update

ChangeLog

• Split tests in D41122
• Improve test long-setcc.ll

ChangeLog

• Sign extend operands as we will want to use the higher bits (they could be rubbish in the previous diff). Add a comment.

We want to do here is to calculate the ADDCARRY / SUBCARRY with a wider type and then use that result to compute the carry/borrow. I think that by sign extension of the operands, the carry/borrow of wider operation should exactly be the same as in a narrower type:

Hi @miyuki I can commit it.

The generic combiner DAGCombiner::visitTRUNCATE does this

• Fix combiner with addcarry that causes PR35103
• Add regression test

This looks sensible to me. Extend the comment in that --E; above at line 2732. If @efriedma or @rengolin do not have further comments I believe this is ready to land.

We believe there is a problem here as the generic combiner presumes we're going to use the first result (0) but actually the code is using the second (1)