In D73904#1874303, @thakis wrote:

I fixed windows tests in a41550cff91b7fb2b56bf0e19ccb341bfd3e37b4 . Please watch bots after landing patches next time.

The function @MaskRay suggested does not address the problem with -march= and others, but it does sound good to use that function. I'm also not sure if -L is a feature that is relied upon at the moment by anybody that uses the baremetal driver. For the moment I'll commit this patch, but a single place across drivers that handles the compiler_rt selection mechanism certainly sounds nice.

Added a test for arm-none-eabi selection
Note that the selection mechanism in the Baremetal driver is rather weak. It does not seem to respond on -mthumb, -march and -mcpu options. The cc1 options show a normalized triple, but the sub-architecture suffix of compiler_rt is the argument exactly as typed on the command line. I'm not going to fix that in this patch, but maybe we want to record this problem somewhere, so we dont forget?

Just noticed the comment from Peter asking for clang/test/Driver/arm-compiler-rt.c, I'll add that in a moment.

With updated tests

I've not read this in detail or followed the list, but I wanted to add that I believe it's important that we have some form of public acknowledgement for the people that have reported security vulnerabilities in there as well.

Committed as r356360

In D58348#1406435, @efriedma wrote:

Is that up for debate in case this is an edge case that is unlikely to be hit?

When it comes to the atomics in general, it's important to allow people to write code with the same performance characteristics as hand-written assembly, or else we force people will go around the compiler using inline asm. But the performance impact here should be basically zero for code that doesn't use an acquire fence, I think, so it shouldn't be an issue here.

In D58348#1404995, @efriedma wrote:

Are you sure that Bugzilla link is right?

Fixed. The bug number in the subject was the correct one.

Follow up patch at https://reviews.llvm.org/D44941

Committed as r328313

Updated the change to take into account f16 types. I cannot test it yet, as f16 seems to be incomplete. I've reported this to SjoerdMeijer already, and he is fixing f16. I am wondering if we can commit this and once f16 constants are actually lowered, update the test as part of that change.

I've raised https://bugs.llvm.org/show_bug.cgi?id=36382 against this. I think the patch to fix it is trivial.

This looks good to me. Thanks

Thanks for the clarification. Looked at the tests and now see what this is doing.
The code looks good to me.
Thanks

On the previous review (D35375), Geoff Berry suggested to do this in DAG Combine, to see if you can do some further combines, and suggested to not limit this to constants: https://reviews.llvm.org/D35375#810045

Code looks well formatted to me, implementation is reasonable to me. It is a bit of a shame that it can't be done in tablegen as the lowering function gets a bit big. But since i128 is not a legal type, I guess this is the best we can get.

Thanks for the hard work. It looks good. Quite an exhaustive list of tests as well.

With the ok of the author, I'll try to get some movement on this work again.

Please make sure that the build directory is not assumed to be present in the root of the source tree. For the rest I'm happy to approve it.

In D35375#808895, @gberry wrote:

In D35375#808771, @mcrosier wrote:

In D35375#808725, @gberry wrote:

Perhaps this would be better done at the IR level, in InstCombine?

Seems reasonable, but we'd have to know this is a good canonicalization for all targets, right?

It seems like it is to me. Worst case (e.g. the target supports 'add' but not 'sub'), the sub would ideally just be transformed back and a negate added during ISel legalization.

Few inline comments on the dead register pass. I've not looked in detail to the other changes done in this patch.

Good idea to add a little write-up on that utility. Might want to make it more clear that it is about the machine scheduler models. It does not do anything for the Itinerary based schedule models.

Sorry for missing the API comments. Thanks for the fix chapuni :-)

Added Alexander as he seems to have touched clang-tidy recently.

This looks good to me. But since I have never touch this code, I'll wait a bit till accepting this for others to have a say.

In D33586#786691, @tschuett wrote:

Is it possible that the tests were excluded from the commit?

In D33586#786362, @ajasty-cavium wrote:

I can commit it on your behalf if you want me to.

Would greatly appreciate it!

In D33586#786349, @ajasty-cavium wrote:

Thank you Chris, I do not have commit rights, should I forward to the mailing list?

OK, it looks good to me now. Thanks.

In D33586#783960, @ajasty-cavium wrote:

Fixed CAS, renamed operands to "old/new" for clarity. Also updated test to use x0/x1 to confirm proper operand order.

Great!

I'll go ahead and commit this on the grounds that it was already approved and the changes I made were the last nit-picks of @rnk

In D33773#783691, @labrinea wrote:

The Address Sanitizer caught a stack-use-after-scope of a Twine variable. This is now fixed by passing the Twine directly as a function parameter.

Found something else.

Sorry, I was too quick. That blacklist is not working properly. Also, it would be good to add some test that show the blacklisting in operation. I just tried an example:

define void @test_atomic_load_add_i32(i32 %offset) nounwind {
     %old = atomicrmw add i32* @var32, i32 %offset seq_cst
   ret void
}

Which should not use WZR, but still uses it with the latest patch.

Looks good to me.
If you could still add a comment in the source on why you blacklist the instruction in the DeadRegisterDefinitionPass that would be appreciated.
Thanks for the nice patch. Looking forward to the patterns for the more relaxed memory models.

LGTM

In D33773#780123, @labrinea wrote:

Actually I was inspired by getPICLabel in ARMAsmPrinter.cpp but I can change it to 'CP' if it makes more sense.

Yes please. Something that makes the connection with a constant pool clear would be helpful for people to make that connection later on.

I was about to approve it but, I have 1 little nitpick that I almost missed. Sorry for the dribble feed. It's about the naming of the global.

The execute-only and PIC features cannot be enabled together. You probably should leave that unreachable in lib/Target/ARM/ARMAsmPrinter.cpp in place. You might even add one under the case ARM::CONSTPOOL_ENTRY in that file for good measure.

This patch is definitely not the right approach. The LEA pseudo instruction should never be selected. I had a bit of a closer look and it seems that (ARMWrapper tconstpool) has various patterns that all match to PC relative pseudo instructions. They all should not happen in XO, so maybe this work is a bit more involved then expected. Also, we still have the ConstantIslandPass that is going to assume these constants pools are present in text and will split basic blocks for no good reason.

@rnk can you let me know if you are happy with the changes?
@rovka I assume the rebase did not change your mind about the patch. But please let me know if you have new reservations.

Rebased onto current HEAD and addressed the comments from Reid.

This refactoring was ready to land some time ago, except for a few small details. It's a shame if we don't commit this refactoring, so with @sanwou01
his agreement, I'm taking over this patch.

This might be a bit more difficult then expected. The code generator assumes the constant pool is PC-rel addressable using ADR at the moment. This is not the case when the constant pool lives in a different section. I should have known, sorry.

In D33586#775434, @ajasty-cavium wrote:

Would prefer to land this patch before adding the weaker ordering, but ST(OP) is dependent on weaker ordering. In any case this should perform equal to, or better than current LDX/STX on hardware that supports it.

Seems like a good start for 8.1-A atomics to me. There are some things that might be improved upon, like supporting weaker orderings. Do you plan on doing that work as well?
Just a few questions inline.

The ARM code generator emits constant pools in a custom way. I believe that is to get them in-line with the code as a constant-pool in .text. But I think that Eli Friedman is right that for this use case it is possible to use the standard LLVM way of emitting them.

In D33586#770042, @t.p.northover wrote:

After further digging, I believe that WZR can't be used for any of the LD<OP> instructions. If you do, you are actually encoding ST<OP>.

That's pretty much true. "ST<OP> ..." is an alias for "LD<OP> [wx]zr" so you're allowed to write both and they mean the same thing, with one caveat: if the instruction has acquire semantics there is no ST<OP> alias for it. You can only write "ldaddal wzr, w1, [x0]" that way, not as "staddal w1, [x1]" for example.

In D33586#769261, @ajasty-cavium wrote:

Understood, this gives us correctness, will also have to ensure deadregister catches ordering and allows wzr for release or weaker.

In D33586#765995, @ajasty-cavium wrote:

In D33586#765646, @t.p.northover wrote:

One other thing you have to be careful of: if an instruction loads into xzr or wzr then it's actually an "STwhatever" operation and defined not to have acquire semantics regardless of its mnemonic. So you probably have to modify AArch64DeadRegisterDefinitionsPass.cpp to ignore these instructions.

Yeah, this was a major concern of mine, technically it's an invalid mnemonic with ladaddal x0, xz, p. One solution was to custom lower all the atomic_load_X where operand 0 wasn't used, and create new AArch64ISD's for them. I wasn't thrilled about this for the first pass (because it seems to work anyway), but for the final it's something to consider.

This landed as https://reviews.llvm.org/rL296454 some time ago. Not sure why it was not auto-closed. Closing manually.

Summary from email conversation:

• GAS redefinitions is considered bad style
• Removing the pre-constructed .text/.bss/.data/... sections from MC is a possible better way, although Rafael did not come back if he likes that approach.

To clarify. This patch fixes some cases where:

clang -S file.c -o - | clang -c - -o file.o

is not identical to:

clang -c file.c -o file.o

because the assembly output 'forgets' some of the flags in the output assembly.

Added tests for ARM and Thumb2 without MOVT support and addressed the inline comments.

Hi Phrakar. I think you mean that there are no processor or OS specific flags with textual representation. But yes, SHF_ARM_PURECODE is the first of its kind. Makes me wonder if we are better of not having a textual representation at all for the flag.

Let me elaborate a bit more on the UB of infinite loops. There existing code lists 2 cases of infinite loops triggered by poison:

In D24651#545945, @sanjoy wrote:

Apologies in advance for this off-the-cuff comment with only having skimmed through your patch, let me know if you've already addressed this: infinite loops are not UB in general, they're UB only if the loop has no side effects (i.e. no volatile or atomic operations and no IO).

Hi Rui.

I've tested it and it removes the issue I saw. The following error is gone:
clang-3.9: error: unsupported option '-fsanitize=address' for target 'aarch64-arm-none-eabi’