User Details
- User Since
- Mar 27 2015, 11:23 AM (303 w, 4 d)
Thu, Jan 14
This doesn't get at the root cause though. Are those labels doing anything in the debug build? Why are they emitted? Can they be reasonably removed?
Looks totally reasonable to me.
Wed, Jan 13
Right, there's no fundamental reason why moving a label has to be forbidden -- but it'd be extremely complex if we allowed moving a label which could cause the re-layout of a fragment we thought we'd already finalized the offsets for. This would happen if the label offset required relaxation of some instruction/data referencing it. That, then, might require undoing the padding from an instruction we've already padded out, due to less alignment-padding being required overall.
Sat, Jan 9
Agreed w.r.t. timing -- I would like to get all of these changes landed soon after the LLVM 12 branch-cut, to allow plenty time to stew on the main branch before they make it into a release to try to identify any issues.
Fri, Jan 8
OK thanks -- abandoning this patch.
Thu, Jan 7
Wed, Jan 6
Fix and test.
I've finally got back to moving this patch forward -- PTAL, thanks!
Tue, Dec 29
Mon, Dec 28
Dec 10 2020
I don't think we should change the meaning of __attribute__((const)) to exclude depending on thread-id.
Dec 9 2020
Dec 1 2020
Oh -- if it's not too late, can you fix the commit message before pushing -- it could probably do with a little more explanation.
Nov 19 2020
LG after fixing the minor nits.
Nov 18 2020
AFAICT, this looks good now. I guess we'll see if anything breaks after it's submitted. =)
Nov 13 2020
Nov 12 2020
Nov 10 2020
Hm, that might indeed be feasible. We'll need to potentially insert padding both before the first type and after the second type, but we can static_assert the correctness against the old layout, so that's not _too_ scary.
Nov 2 2020
Oct 23 2020
LGTM.
Oct 21 2020
Oct 19 2020
Oct 13 2020
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.
Oct 10 2020
Oct 7 2020
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.
Oct 6 2020
Okay. Abandoning this change.
Oct 5 2020
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.
Oct 1 2020
Hm, to start with, the current state of this confuses me.
Sep 29 2020
Sep 24 2020
Sep 23 2020
I'm happy with this now, but please update the commit message to match the updated change.
Sep 18 2020
Comment nits.
Sep 17 2020
Sep 11 2020
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.
Sep 3 2020
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.
Aug 31 2020
Aug 30 2020
Aug 29 2020
Aug 28 2020
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.