• Fixed to return correct token

Created a early exit when a source location overflow is detected,
avoiding further processing or reaching other assert failures.
For that, it was required to create a 'Failure' kind for includes,
as well add return points along the callstack.

• Fixed the destination register class when converting ADDrr to ADDri or ADDspImm. Added test
• Fixed the number of arguments when converting t2SUBspImm to tSUBspi by adding a register at the end. Else, it would break llvm-mca that expects the number of operands to follow the table-gen, even when not used by the instruction.

In D70680#1813841, @dnsampaio wrote:

Looking into CodeGen/Thumb2/thumb2-mov.ll test failure.

Looking into CodeGen/Thumb2/thumb2-mov.ll test failure.

Requested fixes

Requested fixes:

Removed dead/wrong tablegen t2InstSubst, defining a t2ADDri12 using t2_so_imm_neg instead of imm0_4095_neg

• undo some clang-format-diff changes

Added fixme to ARMLoadStoreOptimizer due negative offset

Added test-cases for all added (and fixed existing) thumb2 add/sub alias

Added test

• Fix existing test

Added mir test.
Moved assert before T1 test.

In D70680#1765493, @ostannard wrote:

This looks like multiple bug-fixes in one patch, could they be split up?

Just wondering, I don't fully understand why is that important from the point I do a return FileID();. The critical error has already been inserted to the error list. clang shall either die due an assert in debug, or with a nice message in release. Unless clang is all broken, and the error message would be corrupted, which I don't believe is the case.

Returned tests of invalid sub sp

Re-inserted missing t2SUBri disassemble

Cleared some bits
Added bug test

Fixed adr.w decoding

Refactored emitT2RegPlusImmediate

Git diff 979ae80af7ec49624b932954d22cb91900f17121 did not send a test as well. Feel free to send me a reasonable sized reproducer, the one I have is about 36MB. Don't think it will be that well received.

Yes. It does return a non-valid FileID, and in builds without assert you get the expected error message.

Ping

• Return an invalid FileID instead of exiting.

• Add ", DefaultFatal";

Thanks @RKSimon, will remove the 8 duplicates.

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