In D110582#3040493, @mehdi_amini wrote:

LG, thanks

In D110582#3026567, @bondhugula wrote:

Thanks - a couple of comments.

Fix my typos.

In D109723#2998486, @ftynse wrote:

Thanks!

In D105720#2950863, @MaskRay wrote:

I still don't see a sufficient justification for making the change.

Friendly ping.

On second thought, my earlier analysis regarding the time complexity was not correct. Integrating the implementation of LOCAL directive into the for loop that handles macro arguments does not make it more expensive. This implements LOCAL directive within the for loop and avoids copying the macro body, and hopefully is more consistent with the existing code. This also reduces the number of lines added.

In D105720#2875813, @MaskRay wrote:

In D105720#2875779, @jcai19 wrote:

Yes, the reported case should be fixed by the proposed workaround but sometimes it may be more difficult to find a workaround such as in the the test cases. While this might not be common, PR#45051 shows there still could be real-work use cases of this directive, and I think we should try to support it especially when it does not introduce significant change to the existing code.

The PR doesn't list a use case. This could be an obscure feature which probably no code uses.
(There are now ~80 lines so I am reluctant on accepting that this is insignificant amount of code.)

In D105720#2872322, @MaskRay wrote:

DenseSet<CachedHashStringRef> may be better than StringSet. The former doesn't need to duplicate the string.

Use StringSet instead of std::unordered_set <std::string>.

In D103184#2831542, @nickdesaulniers wrote:

Thanks for following up on a fix!

After chatting with @nickdesaulniers offline, we have decided to abondoned D104656 in favor of keeping all the updates to this differential review for consistency. Appologies for any inconvenience caused.

In D104656#2831034, @nickdesaulniers wrote:

This reverts commit fd11a26d368c5a909fb88548fef2cee7a6c2c931

Shouldn't that be This relands commit fd11a26d368c5a909fb88548fef2cee7a6c2c931?

Add comments explaning the difference with D103184. The implementation in D103184 unconditionally calls getAArch64ArchFeaturesFromMarch, which returns false when an irrelevant -Wa value is detected and parsed (e.g. -Wa,--version) and causes Clang to bail out.

Use the original flawed code for future reference.

It's probably clearer to use a new differential review for relanding. I've created https://reviews.llvm.org/D104656. Sorry for the noises.

The original implementation was indeed flawed. The failed test does not specify target triple so on X86 machines the issue won't happen unless I explicitly specify "-target aarch64".
This fixes the problem.

In D103184#2803768, @thakis wrote:

FWIW the failure goes away locally if I revert this change here, so it's definitely due to this change here.

Things have been red for a while, probably good to revert while you investigate by this point.

In D103184#2803513, @thakis wrote:

This breaks tests on arm macs: http://45.33.8.238/macm1/10931/step_7.txt

Please take a look and revert for now if it takes a while to fix.

In D103184#2799732, @nickdesaulniers wrote:

That looks better, thanks @jcai19 . Based on @DavidSpickett 's LGTM I think this is now ready to land.

@DavidSpickett did ask:

Also please include in the commit message the "rules" that we're using here for parsing. Like I did for the arm change.

So you may want to edit the commit description manually in phab if you plan to arc patch this in before merging, otherwise please do so locally before merging to main then pushing.

Update test cases based on comments.

In D103184#2798398, @DavidSpickett wrote:

LGTM; any additional thoughts @DavidSpickett ?

A couple of additional tests just to be safe but otherwise LGTM.

Also please include in the commit message the "rules" that we're using here for parsing. Like I did for the arm change.

Address the comments and add more tests.

Add a test case.

In D103184#2793055, @DavidSpickett wrote:

$ cat /tmp/test.s
irg x0, x0
$ aarch64-unknown-linux-gnu-as -march=armv8.5-a+memtag -march=armv8.1-a /tmp/test.s -o /dev/null
/tmp/test.s: Assembler messages:
/tmp/test.s:1: Error: selected processor does not support `irg x0,x0'

GAS is also taking the last value of march. So if we were to follow clang Arm's behaviour, the result would be the same.

Except we don't have to rely on the llvm backend picking the last of "target-feature". Which probably works for architecture versions but for individual extensions I don't think so.

If we make it additive this could happen:

-march=armv8.5-a+memtag -march=armv8.2-a

Becomes: (mte is the backend name for memtag for silly reasons)

-target-feature +v8.5-a -target-feature mte -target-feature +v8.2-a

So now instead of getting v8.2 only, the user now has v8.2+memtag. Only if they do armv8.2-a+nomemtag will they get what they expect (well, what I'd expect). Which means they'd need to know every previous march value.

Limit the change to assembler files, and add more tests.

Thanks for the comments. Please see my replies inlined.

This broke the ppc64le-sanitizer builderr: https://lab.llvm.org/buildbot/#/builders/19/builds/3565. Will fix it.

In D98916#2678792, @peter.smith wrote:

Thanks for the update LGTM. It sounds like you have all the cases covered. Apologies for the noise.

Fix a typo.

In D98916#2673445, @peter.smith wrote:

I have a one major concern, and a number of minor nits that could easily be addressed prior to commit if necessary.

The major concern is that if the intent is to just permit expressions for Arm state LDR instructions (addition of one fixup) then a lot of code in the AsmParser looks like it is permitting expressions for Thumb and Thumb2 LDR instructions that wouldn't use the one new fixup. Can you double check the predicates to make sure that they apply to the instructions you want to permit expressions on? It is possible I've got something wrong though. If I'm right then where we would have had errors had someone use an expression then we may get an internal error or crash as the backend won't be able to handle the expression. My suggestion would be to take out from ARMAsmParser all the code permitting exceptions except for the ARM instructions that support the fixup. Adding support for expressions to all the ARM and Thumb instructions is possible but that would probably be better done over several patches. If I'm wrong please ignore me and sorry for the noise.

Address comments.

I'm ok with this, and thanks for taking the time to work on it. It would be good to collect another LGTM from another reviewer before submitting.

Only accept MCExpr in the affected instructions.

Relanded without -fno-initegrated-as case at https://reviews.llvm.org/rG76d9bc72784d88f4dd57b9939e52c73739438af5.

We can probably remove the -fno-integraetd-as case.

Update tests.

Update tests to address comments.

Address comments.

In D98916#2637305, @peter.smith wrote:

The fixup code looks reasonable to me as it is mostly reusing the same logic as the pc-relative fixup. The names are internal to LLVM so there isn't any particular standard. I think this fixup would correspond to the relocation R_ARM_ABS12 https://github.com/ARM-software/abi-aa/blob/master/aaelf32/aaelf32.rst#relocation-codes so it may be worth using fixup_arm_ldst_abs_12 but that is only a suggestion. I wouldn't expect the assembler to use the relocation as the range is so short that any attempt to use it would likely result in an out of range error.

Address comments so far.