ChangeLog:

• Updated the patch to use ADDCARRY / SUBCARRY nodes.

Ok, Chrome took a bit to build with a debug compiler but finished successfully.

This LGTM, but wait a couple of days before comitting in case @rsmith or @scanon (or others!) have further comments.

In the second instance of PR34045 what happens is that ADDC is exactly HiAdd, not just a predecessor of the latter.

ChangeLog:

• fix for the second instance of PR34045
• also integrates the changes to fix PR34564, fall-out of the original change, that was reverted as well

We're still hitting PR34045. I also plan integrate the fix in D37690 to ease reverting if needed.

ChangeLog:

• Use MVT::i32 instead of VT

This blunder of mine caused PR34564.

Thanks for the review Eli!

ChangeLog:

• Fix problem in S/UMLAL combiner that caused PR34045.
• New bugpoint-reduced test based on the reproducer of PR34045.

This LGTM now. Wait a couple of days in case @rsmith has more comments.

There are a few tests checking the precise interaction between __fp16 and _Float16 values but I don't see tests interacting _Float16 and the other standard floating types float, double and long double.

Reopening: this is causing PR34045.

I plan to submit this tomorrow morning (BST time).

ChangeLog:

• clang-format fixes

ChangeLog:

• Remove unnecessary combiner for ARMISD::SUBC
• Refactor a bit the conversion carry to/from boolean

ChangeLog:

• Use ISD::SUB instead of ARMISD::SUBC when inverting the borrow / carry.
• Due to the above, remove now unnecessary target-specific combiner.

Hi @efriedma thanks for your suggestion, this way I can avoid one target-specific combiner which I have now removed.

Hi @efriedma, thanks for the review, please find the updated changes. I was wrongly handling ISD::SUBCARRY because its operand is a borrow while ARMISD::SUBE expects a carry.

ChangeLog:

• Correctly handle the second operand of ISD::SUBCARRY. We need to invert its value (i.e. 1-x) before passing it to ARMISD::SUBE, also the result of ARMISD::SUBE has to be inverted again to match what ISD::SUBCARRY means.
• Fix USUBO lowering as a consequence of the above.
• Add a combiner for (SUBC 1, (SUBC 1, X)) → X
• Compute the known bits of (SUBC 1, (ADDE 0, 0, C)) (1 bit) to avoid spurious and instructions.
• Update the tests for usub_builtin_overflow with the correct code generation.
• Remove combiners that became pointless after the changes above.

Thanks again @efriedma.

ChangeLog:

• Remove incorrect computation of known bits for an operand that contains a copy of the CPSR and not a boolean.

Hi @efriedma, thanks again for the comments. I have updated the diff with your suggestions.

ChangeLog:

• Use a single file for the new tests instead of scattering them in two files.
• Fix a bug introduced when ARMTargetLowering::computeKnownBitsForTargetNode was updated.

Hi @efriedma, thanks for the review. I have updated with your suggestions.

ChangeLog:

• Fix combiners so they preserve the arity of the result.
• Remove bogus combiner.
• Add new tests for {u,s}{add,sub} overflow for Thumb and Thumb2

ChangeLog:

• clang-format LowerUnsignedALUO
• fix FileCheck test

Hi Eli,

Hi @ogoffart thanks for fixing this.

thanks for pointing me to the ADDCARRY/SUBCARRY nodes. I tried to use them but I ran into serious problems (maybe due to some inexperience on my side working with SelectionDAG). First if I want to use them I assume I have to make them legal for the ARM back end, this means that I have to lower them somehow. After the short discussion in the list about the semantics of SUBCARRY I made ISD::ADDCARRY to lower to ARMISD::ADDC and ISD::SUBCARRY to lower to ARMISD::SUBC plus doing an extra (ISD::ADD 1, c) to the carry result, to change from borrow semantics (ISD::SUBCARRY semantics) to ARM carry semantics.

This LGTM. Thanks for the refactoring and the fix.

I agree with the spirit of the change but I think you want to use AArch64:: enumerators instead of those in ARM::

Thanks Momchil, this looks sensible to me.

Given that we are already considering extensions, maybe you want to add a test for compound statements in expressions.

This LGTM, @marsupial what do you think?

After the docs update this looks good to me.

This looks good to me.

Changelog:

• Fix formatting.
• Emit __unaligned in the right order (as defined by the Itanium ABI) when there are Objective-C++ ARC vendor qualifiers.
• New test for __unaligned and ARC's __weak, __strong and __autorelease

Fix formatting of tests.

I think what happens is that the implicit declaration is done in the prototype scope which is not visible later.

I wonder what should we do in C99 for cases like

This clang change looks sensible to me. Can you add documentation in LanguageExtensions.rst describing this new pragma and its semantics? Thanks!

This supporting LLVM change looks good to me.

Thanks @aaron.ballman for the review.

Ok, I will revert this one. @chill will suggest another solution. Sorry for the fuss.

Abandon the old approach which cannot possibly work and just opt to mangle __unaligned.

clang::Sema::IsOverload explicitly forbids the __restrict case for the qualifier of non-static member functions. I assume __unaligned is not forbidden because the MSVC ABI does encode this qualifier while the Itanium ABI currently does not.

A somewhat obvious comment: no chance for this to be accepted without LIT tests. I understand you have your doubts on the best approach to testing -- and it's OK to ask either here or on llvm-dev -- but tests should be added nevertheless.