In D71726#2207700, @yaxunl wrote:

clang does not always emit atomic instructions for atomic builtins. Clang may emit lib calls for atomic builtins. Basically clang checks target info about max atomic inline width and if the desired atomic operation exceeds the supported atomic inline width, clang will emit lib calls for atomic builtins. The rationale is that the lib calls may be faster than the IR generated by the LLVM pass. This behavior has long existed and it also applies to fp atomics. I don't think emitting lib calls for atomic builtins is a bug. However, this does introduce the issue about whether the library functions for atomics are available for a specific target. As I said, only the target owners have the answer and therefore I introduced the target hook.

LGTM.

In D89615#2342857, @vitalybuka wrote:

Why we can't just switch to elf TLS?

In D88659#2339357, @Xiangling_L wrote:

Hi @jyknight , are you okay with us changing the "definition" of SuitableAlign without sending a note to the mailing list?

I have tried adding the expression folding stuff to`MCExpr::evaluateAsRelocatableImpl` but I abandoned that idea because evaluateAsRelocatableImpl does not like a modifier different from VK_None (@PLT). (What does a@plt + 1 do? Relaxing the condition (== VK_None) may work... but the semantics are not very clear.

In D88659#2319636, @hubert.reinterpretcast wrote:

In D88659#2318203, @jyknight wrote:

It seems like on AIX, __BIGGEST_ALIGNMENT__ should just be set to 16, then. I'm not sure why you want it to be 8?

/// Return the alignment that is suitable for storing any
/// object with a fundamental alignment requirement.

Vector types have extended (not fundamental) alignment on AIX, because malloc is not guaranteed to return addresses that are 16-byte aligned.

As you mentioned, without this patch, SuitableAlign is used for the predefined __BIGGEST_ALIGNMENT__ and alignment for alloca. But on AIX, the BIGGEST_ALIGNMENT should be 8bytes, alloca and __attribute__((aligned)) should align to 16bytes considering vector types which have to be aligned to 16bytes, that's why we want to split SuitableAlign following current Clang state. We'd like to split something out to consider vector type alignment.

Okay. Abandoning this change.

I disagree. You'll be inserting an instruction that won't be executed on the default path before the asm goto block. While this may be okay in a majority of cases, I don't suspect that it'll be good to rely on it being always okay.

When we go to resolve this PHI node, there's no place to put the put the values that works in all situations; we can't split the critical edges and it's not valid to place the resolved instructions at the ends of the predecessor blocks.

Hm, it doesn't seem like it should be a problem to duplicate an inlineasm_br instruction. I'm interested to see what's going wrong here.

Hm, to start with, the current state of this confuses me.

In D88195#2293597, @nickdesaulniers wrote:

In D88195#2293589, @void wrote:

Clarify commit message.

Phabricator unfortunately won't amend the review description when the commit message is updated; you'll need to manually edit the description via phab's UI for other reviewers to observe the update. :(

I'm happy with this now, but please update the commit message to match the updated change.

Comment nits.

But this (re-)breaks the functionality of -fvisibility-inlines-hidden on Darwin. That seems bad? I'd've liked to see more of an explanation as to why this was considered a necessary breakage.

Do you have open questions on whether some callsites passing "false" here, should be switched to true? Given what's here, I would say that it definitely does not makes sense to add this parameter everywhere.

ro: In this case, I would say to defer to Paul, although I wish that he would change his mind.

BTW, to the apple folks here, it'd sure be awesome if this could be backported into XCode 12's clang. :) (c.f. FB7037642).

In D86142#2234882, @MaskRay wrote:

In D86142#2234472, @sfertile wrote:

In D86142#2225679, @psmith wrote:

Given that BFD supports this behaviour but Gold and LLD do not I suspect that the risk of breaking existing programs is small, especially as LLD tends to be used with more recent programs where -fno-common is the default. However I think we should be cautious and perhaps enable under a command line option, especially as it looks like this is a convention rather than part of a specification. If anyone has better references, then I'm happy to retract.

I agree the new behavior needs an option to enable it and disable it. Initially I considered having the Fortran driver pass the option to LLD, but after discussing it offline with @rzurob I think it might be best to enable the behavior by default. The consumer of these libraries aren't necessarily Fortran programs, and as you mentioned its likely to be lower risk because we are adopting a behavior already implemented in ld.bfd.

There are 4 combinations: (ArchiveFile,LazyObjFile) x (ELF, BitcodeFile). LazyObjFile is for --start-lib/--end-lib. Do you think whether it is worth making LazyObjFile or BitcodeFile consistent with ArchiveFile x ELF behavior? If we add an option (say, --fortran-linking, the Dec 1999 binutils thread suggested but did not adopt), we will have better justification that we don't make LazyObjFile or BitcodeFile consistent.

In D86260#2229406, @void wrote:

In D86260#2229387, @jyknight wrote:

The commit message is confusing for this -- this isn't a correctness fix, but a minor optimization, right?

I'm not convinced it's just an optimization, but I could be swayed. It seems to me that the value coming into the PHI shouldn't rely upon the semantics of the asm block. So for instance would this be incorrect?

The commit message is confusing for this -- this isn't a correctness fix, but a minor optimization, right?

There is going to be a bunch more complexity required here, I'm afraid.

Presumably this is a bug in something -- either the solaris linker, in g++, or in the llvm code being mis-compiled? It seems unfortunate to put in place a hacky workaround like this, without a bug reference to the responsible component.

Oh, one more note, C11 has -- and clang already supports -- _Atomic long double x; x += 4; via lowering to a cmpxchg loop. Now that we have an LLVM IR representation for atomicrmw fadd/fsub, clang should be lowering the _Atomic += to that, too. (Doesn't need to be in this patch, but it should be done.)

In D71726#2182667, @tra wrote:

If a target would like to treat single and double fp atomics as unsupported, it can override the default behavior in its own TargetInfo.

I think this new header ought to go in include/llvm/Bitcode/, not Bitstream?

Right, the trivial_abi attribute doesn't guarantee that you pass an object in registers, it just gives permission for the implementation to treat the struct as-if it was a trivial struct. On ARM32, since the ABI specifies that trivial structs > 4 bytes are returned indirectly by passing a hidden pointer to a pre-allocated return struct, that's what you get.

In D71726#2165445, @yaxunl wrote:

In D71726#2165424, @jyknight wrote:

Why not have clang always emit atomicrmw for floats, and let AtomicExpandPass handle legalizing that into integer atomics if necessary, rather than adding a target hook in clang?

Not all targets can legalize fp atomics by AtomicExpandPass. Some targets need library support.

Why not have clang always emit atomicrmw for floats, and let AtomicExpandPass handle legalizing that into integer atomics if necessary, rather than adding a target hook in clang?

This kind of change is usually ok to just push, with a note on the original review that you had to push a followup fix.

Improve commit message wording
Simplify added test a bit

FWIW, it rather bugs me when tools print their entire (long) help upon command-line syntax error. Simply printing an error is generally better. (E.g. "error: no input files specified.")

INLINEASM_BR already returns true for hasUnmodeledSideEffects(), so the diff above doesn't change anything.

It looks like the issue shown in this test-case appears in Two-Address instruction pass, not Machine Sink.

Of course, running into the problem also requires an asserts-enabled build of clang, because this change has triggered a Clang bug, which will need to be fixed before this can be recommitted.

BTW, I just noticed recently that we have a -mlinker-version= flag, too, which is only used on darwin at the moment. That's another instance of "we need to condition behavior based on what linker we're invoking", but in this case, between multiple versions of apple's linker, rather than which brand of linker. That doesn't impact this directly, but just thought I'd mention it as it's in the same area of concern.

It's odd to have CreateAtomicCmpXchg and CreateAtomicRMW not have Align as an argument when the constructors do, but as long as the migration is finished in an upcoming commit, this seems fine as a step on the path.

Thanks for the cleanup!

I suggest removing the extraneous level of directory "Darwin" -- just put the files in "llvm/tools/llvm-libtool-darwin", name the source "llvm-libtool-darwin.cpp", and say you're adding "llvm-libtool-darwin" in the commit message.

Minor fixups.

After working on the previously mentioned unreachable-block-end cleanup for a bit (not done yet), and thinking about this patch more, I actually think it's okay to go ahead with this change now. The potential problem I was worried about is that the code may think that if you have INLINEASM_BR in a basic-block, where the block-end is unreachable, and the block is followed by a basic-block which is one of the indirect targets of the INLINEASM_BR, the code will think the first block falls through, when it does not.

Differences in new patch, since previous one:

• Folded in void's change (but I made it in the isSchedulingBoundary() functions, rather than the two callers.
• Marked the INLINEASM_BR instruction as always having unmodeled side-effects, rather than only if it's marked thus. (not to fix any known miscompilation -- clang always marked it as such anyhow.)
• Removed FIXME comments, per review comments.

In D81607#2090768, @efriedma wrote:

Disclaimer: I don't plan to rework callbr's operands before landing this. Was that a conditional LGTM?

Wasn't intended to be conditional on any code changes. Just that we don't plan to ever allow jumping to blockaddresses passed in as register operands.

I assume there's something important @fhahn was alluding to, but I'm not sure precisely what, and I would like to make sure everyone's happy with this.

The reason we can't split edges coming out of indirectbr instructions is that in general, there is no way to fix the related blockaddresses in functions that contain multiple indirectbrs. We can't rename the successor in one indirectbr without renaming it in all of them. And if we rename it in all of them, the edge is still critical after the transform.

We want to be sure the same issue doesn't apply to callbr instructions. If it does, then we would need to fix the callers of SplitCriticalEdge, not SplitCriticalEdge itself.

Just rebase. (Doesn't address review comments)

This isn't sufficient to verify that atomic.c actually works properly (which, BTW, it does not). But at least verifying that the basic single-threaded functionality semantics hold is a good start.

LGTM.

The current state doesn't appear to block it or prevent it in any way

We already _aren't_ and _cannot_ be compatible with cctools strip's command-line, because cctools strip already has a bunch of one-letter option choices which actively conflict with existing options in llvm-strip (and GNU strip from which we copied them). There's at least -s -R -d -N with conflicting meanings.

Isn't this potentially problematic from a compatibility point-of-view, since it squats a single-letter command-line argument for a mach-o-specific feature, while llvm-strip is generally trying to be compatible to GNU strip?