- User Since
- Mar 27 2015, 11:23 AM (287 w, 2 h)
Wed, Sep 23
I'm happy with this now, but please update the commit message to match the updated change.
Fri, Sep 18
Thu, Sep 17
Fri, Sep 11
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.
Thu, Sep 3
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.
Mon, Aug 31
Sun, Aug 30
Sat, Aug 29
Fri, Aug 28
ro: In this case, I would say to defer to Paul, although I wish that he would change his mind.
Aug 25 2020
BTW, to the apple folks here, it'd sure be awesome if this could be backported into XCode 12's clang. :) (c.f. FB7037642).
Aug 24 2020
Aug 20 2020
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.
Aug 18 2020
Aug 17 2020
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.
Aug 10 2020
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.)
Jul 29 2020
Jul 27 2020
I think this new header ought to go in include/llvm/Bitcode/, not Bitstream?
Jul 21 2020
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.
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?
Jul 20 2020
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.
Jul 17 2020
Jul 16 2020
Jul 14 2020
Jul 13 2020
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.
Jul 10 2020
It looks like the issue shown in this test-case appears in Two-Address instruction pass, not Machine Sink.
Jul 6 2020
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.
Jul 1 2020
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.
Jun 29 2020
Jun 26 2020
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.
Jun 17 2020
Jun 15 2020
Jun 12 2020
Just rebase. (Doesn't address review comments)
Jun 9 2020
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.
Jun 8 2020
Jun 6 2020
Jun 5 2020
May 27 2020
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.
May 26 2020
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?
May 24 2020
May 21 2020
In the review on the prerequisite patch, it turned out there's another thing that needs to be fixed first before this -- disambiguating whether the end of a block falls through, or is actually unreachable.
It's worrying to me that there number of places in LLVM that at the exact argument value of "-fuse-ld=". E.g. in the windows and PS4 toolchains. We already claim to support arbitrary values and full paths, but if you specify "-fuse-ld=/path/to/lld-link" on Windows today, you end up with different behavior than "-fuse-ld=lld" (which gets translated to searching for an "lld-link" binary, but also triggers other conditions).
May 20 2020
In the commit message, you refer to the preferred alignemtn as the "true" alignment, but that's misleading. As discussed on the mailing list thread, it's not true alignment at all.
May 15 2020
Oops, actually upload changed version. :)
Handle the case where the apparent fallthrough is not actually a
fallthrough, because the end of the block is unreachable.
May 13 2020
Are you planning to land this or D79793 is the alternative approach?
May 12 2020
It looks like you didn't squash your two commits before uploading, so the diff for review now only includes the changes for the comment, not the complete patch. Other than needing to squash the commits, LGTM.
Is the endgame here that we relax the restriction that updateTerminator() requires an analyzable branch?