LGTM. Could wait a day before committing in case anyone else has a say.

Hi,
this seems mostly good, perhaps just missing a disassembler test, such as in llvm/test/MC/Disassembler/AArch64/armv8.4a-mpam.txt?

Attending review request:

Indeed our main concern is regarding the access widths of loads. As mentioned by @rjmccall, most volatile bitfields are used to perform memory mapped I/O, and some hardware only support them with a specific access width.
The spurious load I am more than glad to leave it disable behind a command flag, so it will only appear if the user requests it. See that volatile accesses might have side effects, and for example, an I/O read counter holding an odd number could define that the data is still being processed.

Hi @jfb. In a example such as:

struct { int a : 1; int b : 16; } S;
extern int expensive_computaion(int v);
void foo(volatile S* s){
  s->b = expensive_computation(s->b);
}

There is no guarantee that s->a is not modified during a expensive computation, so it must be obtained just before writing the s->b value, as a and b share the same memory position. This is already done by llvm. Indeed, the exact output would be

define void @foo(%struct.S* %S) local_unnamed_addr #0 {
entry:
  %0 = bitcast %struct.S* %S to i32*
  %bf.load = load volatile i32, i32* %0, align 4
  %bf.shl = shl i32 %bf.load, 15
  %bf.ashr = ashr i32 %bf.shl, 16
  %call = tail call i32 @expensive_computation(i32 %bf.ashr) #2
  %bf.load1 = load volatile i32, i32* %0, align 4 ; <<<== Here it obtains the value to s->a to restore it.
  %bf.value = shl i32 %call, 1
  %bf.shl2 = and i32 %bf.value, 131070
  %bf.clear = and i32 %bf.load1, -131071
  %bf.set = or i32 %bf.clear, %bf.shl2
  store volatile i32 %bf.set, i32* %0, align 4
  ret void
}

@ostannard might prove me wrong, but according to the AACPS:

When a volatile bit-field is written, and its container does not overlap with any non-bit-field member, its
container must be read exactly once and written exactly once using the access width appropriate to the
type of the container. The two accesses are not atomic.

This rule does not define that the load is done if required. It states that it will be read once. It even gives the example that an increment will always perform two reads and one write, bitwidth agnostic. It writes just after:

Note: Note the volatile access rules apply even when the width and alignment of the bit-field imply that
the access could be achieved more efficiently using a narrower type. For a write operation the read must
always occur even if the entire contents of the container will be replaced.

The rationale is to provide a uniform behavior for volatile bitfields independent of their width (as far they do not overlap with non-bitfields).

This patch could hack clang to generate an extra load. However, my knowledge in the clang code base is not extensive. How could we ensure that the width of loads and stores are the size of the container, and that they don't overlap non-bitfields?

LGTM. Thanks. Will commit for you as requested soon.

• Fix comment

Hi, I do agree that giving the user a warning that the argument is ignored is the best solution. If you wouldn't mind adding it to this patch, that would be great. Thanks.

clang -### -target arm-arm-none-eabit -march=armv8-m.main+crypto did not show +sha2 or +aes. After the patch it does.
I believe that is not expected, as in ARM.td crypto is not applied for any M profile. And Arm®v8-M Architecture Reference Manual does not reference these extensions neither.

LGTM. One optional nit as it is not related with this patch anymore.

• Avoid appending __noswap_ to intrinsics that are BigEndianSafe
• Moved to monorepo

Breaks the header. Needs to avoid generating calls to functions with predicated __noswap when it is BigEndianSafe.

• Consider BigEndianSafe intrinsics that all inputs and outputs are scalar or single element vectors

Hi @krisb,
thanks for looking into this, and sorry for the delay, was out for a week.

In D63152#1628975, @wuzish wrote:

Is there any fix patch proposed to track/fix the regression? @dnsampaio

Just seen that this breaks clang/test/Sema/struct-packed-align.c, working on it.

I have tested this in our MacOS and linux environments. @thakis @thegameg @phosek, would it be possible for you to check if this works for you?

Fix test

Hi, first thanks for those that looked into this and sorry for the delay.
We have investigated the errors and seems that the test was, first in the wrong folder, inside CodeGen where it should be in CodeGenCXX and we should use clang_cc1.

True. Thx again.

• Joined assignments for default alignments and neon_vector alignment
• Added missing align 8 test

• Joined assignments for default alignments and neon_vector alignment

Set android-abi default to 128. Added tests for android and not-android.

• Set androideabi alignment to 128 bits

Was fixed just after if the include <atomic>

I am getting this error with gcc 8.2

/work/bf/LLVM/local/src/lib/Support/PrettyStackTrace.cpp:57:39: error: variable ‘volatile std::atomic<unsigned int> GlobalSigInfoGenerationCounter’ has initializer but incomplete type
 static volatile std::atomic<unsigned> GlobalSigInfoGenerationCounter =
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

• Fix previously existing tests

• Added tests

In D64210#1570515, @LukeCheeseman wrote:

Are there some changes/addition to tests attached to this?

Perhaps this makes it clearer:
https://rise4fun.com/Alive/4TLv

Hi @lebedev.ri,
Nice you looked this one as I am not quite sure what to do about it. Any suggestions?

Fixed inline requests

Already part of D63152

Thanks for the reviews. I'm changing the new test to take a mir obtained just before shrink-wrap pass and validating just that one pass.

Hi @thegameg,
My initial thoughts on how to optimize this was to use alias analysis to check that load and stores for sure do not alias with the stack of the current function. It seems that the current api accepts two memory operands, and then turns them into memory regions. I don't know if it possible, as we might not know the sp value it self, but my idea was to create a function that accepts a memory access and a memory region, which would be set to [SP, SP + stackSize].
In pr42136, @eli.friedman also mentioned that might be quite simple to prove that a load accesses a global variable, as well possible to prove that the address of some particular stack objects doesn't escape (also pointed by @chill) .