In D87114#2280184, @Xiangling_L wrote:

Do we want to take advantage of this patch to move those checks into the cmake configure so we don't need to duplicate the logic in 2 files as @sfertile mentioned previously?

• Fix whitespace and indentation

In D87113#2266807, @hubert.reinterpretcast wrote:

since we will be building both 32-bit and 64-bit compiler-rt builtins
from a single configuration.

@daltenty, from the limited context of this patch, this sounds questionable in terms of avoiding silent errors now and in the future. If the idea is to configure once and then run twice in the same build tree, then some things may get cached either in CMake or in generated headers, etc.

• Adjust so we don't alter the MVS case

LGTM

• Add AR flags to the builtins CMake too since it may be used as a toplevel by the runtimes build

• Fix comment.

Fixed variable expansion syntax in TOOLCHAIN_TOOLS

In D85329#2198203, @hubert.reinterpretcast wrote:

@daltenty, if these tools happened to be on the user's $PATH, then would they get picked up? Is that the desired behaviour?

(summarizing offline discussion)

LGTM, thanks!

LGTM, thanks!

LGTM, thanks!

LGTM

LGTM

• Use symmetrical constants instead

LGTM other than minor nit

In D82827#2122786, @daltenty wrote:

In D82827#2122013, @jhenderson wrote:

In D82827#2121798, @ikudrin wrote:

The patch looks appropriate, but now I think that maybe it would have been enough to simply use symmetrical constants for these particular tests?

Testing endianness behaviour of the code under test is not what the original change was about, so getting rid of this byte-swapping code entirely would make the testing simpler, I agree. @daltenty, are you happy to do that? It should be a fairly straightforward change, I believe.

That makes sense to me, I will update to remove the byteswap and add a set of symmetrical constants for the big-endian case.

• Use an Optional big-endian output string rather than trying to byte swap

In D82827#2122013, @jhenderson wrote:

In D82827#2121798, @ikudrin wrote:

The patch looks appropriate, but now I think that maybe it would have been enough to simply use symmetrical constants for these particular tests?

Testing endianness behaviour of the code under test is not what the original change was about, so getting rid of this byte-swapping code entirely would make the testing simpler, I agree. @daltenty, are you happy to do that? It should be a fairly straightforward change, I believe.

This fix has some problems on cross-compiling builds (i.e. big endian host targeting little), created D82827

Confirming wrapper LGTM

LGTM other than minor nit

• Address comments round 1
  • Use numeric_limits for static assert
  • Use bit mask interpretation for VirtualRegFlag
  • Add range assert for index to virtual reg conversion

LGTM apart from minor nit

LGTM

In D81120#2086157, @hubert.reinterpretcast wrote:

In D81120#2085913, @jasonliu wrote:

xlclang alias.c -c
alias.c:2:35: warning: 1540-2990 The attribute " attribute((alias("oldname")))" is not supported. The attribute is ignored.
extern int newname attribute((alias("oldname")));

Do we know whether the macro is used on variables?

Based on the result of the pre-merge checks, do we need to include the clang change in the scope here?

LGTM

LGTM

• Fix directive spacing

• Move directives in AsmInfo into base class order

I'm sure this may have been considered, but given that .byte also supports a limited string syntax (given with the limitation that it can support ), is there a reason we can't just glue consecutive runs of printable characters together?

• Update comments
• Add unpacked struct case and add tests
• Fix asm whitespace and block spacing in tests

LGTM