- User Since
- Jan 7 2013, 9:35 AM (332 w, 21 h)
Thu, May 16
This is great! Might it be worth mentioning LLVM_INSTALL_TOOLCHAIN_ONLY somewhere?
Tue, Apr 30
Wed, Apr 24
Apr 12 2019
I assume the linker doesn't need to do anything with this, it can just regenerate it after linking?
Apr 11 2019
Apr 4 2019
I guess the lld code here is an even better answer to my question on the tool-conventions review: the linker behavior is exactly the same.
Apr 2 2019
I think these are leftovers from when we were using ELF.
Mar 29 2019
Mar 28 2019
Mar 27 2019
Reverted in rG039be787914610c28cba45c4557454e0a96939ab. Caused a strange error with the waterfall sysroot's build of libcxx: https://logs.chromium.org/logs/wasm/buildbucket/cr-buildbucket.appspot.com/8917800786005174656/+/steps/libcxx/0/stdout
Mar 26 2019
Mar 25 2019
still LGTM. In a followup CL it should be straightforward to add back fast-isel and you might be able to re-use the same checks in load-store-pic.ll. (but you wouldn't need to add fast-isel+PIC to address-offsets.ll because that's testing an optimization that fast-isel doesn't do.)
Mar 19 2019
Mar 18 2019
to make the change even clearer, you could just say in the commit message that it just adds the missing non-equality opcodes.
LGTM; I wonder if it makes sense to have predefined macro for the C++ tag (or perhaps just a regular macro for use in libcxxabi?)
Mar 15 2019
wow that's a very large meme.
Mar 14 2019
LGTM assuming that comment is right. Assuming it's correct, I don't really understand what exactly ExternalSymbol actually means though.
Mar 12 2019
So by "correctly" you mean that the TRY goes before the EH_LABEL rather than between the label and the call, and the CATCH goes after the label rather than at the top of the BB?
Mar 7 2019
Mar 5 2019
LGTM with the comment suggestion.
Feb 26 2019
Feb 25 2019
LGTM, this is a nice improvement.
Feb 22 2019
Makes sense. I think the thing that has changed compared to when we started is that symbol types like function, global, and event are more first-class in the wasm object format. Anyway, this change seems fine. I assume the asm parser already isn't using these annotations and we still round-trip?
Is EHPadUnwindMap analogous to any of the similarly-named data structures for WinEH? If so, that would seem to make it even less likely to accidentally break, since any change that breaks it would also break WinEH.
You mention that this is a bugfix. what's the bug that is fixed? An optimization not keeping the map up to date?
IIRC this code goes back to when we were piggybacking on ELF, and is more-or-less there to match how those variants are used on ELF. I don't see any asm parser changes in this CL, but I could imagine that they would exist so that the assembler could know what kind of reloc to use even without any context or special knowledge of symbol types; does this change affect how asm is parsed? Maybe we've already added enough extra intelligence to the assembler that we don't need them.
Feb 21 2019
Feb 15 2019
By "pick one at random" you mean we are rewriting one of the calls to drop or add parameters, right? And the one we pick is just the first one we happen to encounter?
Feb 12 2019
Without the context of MC here there's no indication what kind of flags the .init_flags directive refers to. Maybe call it init_segment_flags?
Feb 7 2019
@quinripa IIUC this patch was committed a year ago, so these symbols should be working?
Yeah, based on our conversations (e.g. in D57874) we should go ahead with this.
Oh I guess another option would just be to pin all 3 flags together here, but since -pthread sets a preprocessor define and may also affect linker behavior, I think it's fine to allow atomic codegen without setting -pthread.
So this CL has the effect that setting -pthreads will also set -matomics.
Currently as you mentioned we have the problem that we can't make our current logic of "do we lower away the atomics" be controlled by the target features because it's done at pass config time and not codegen time (where there's no access to the per-function subtarget).
Also IIUC on ARM, it's -mthread-model that controls generation of atomic instructions, so it makes sense that -mthread-model does the same thing for Wasm. But it's also true that for wasm we use -m<feature> to enable codegen for a particular wasm feature, so it would be good to make -matomics do the same for consistency.
So, can we just pin those 2 flags together here? i.e. setting one will just cause the other to be set?
(I guess we'd also need consistency check so that if someone does something like -mthread-model=posix -mno-atomics we either throw an error or make it so that one of those always overrides the other).
Feb 6 2019
Does this affect llvm-objdump or just llvm-readobj, or both (I don't see any tests here with llvm-objdump but the CL description mentions it)
I think we should change the logic in the backend to use the atomics target feature rather than the thread model to determine whether to run the LowerAtomics pass. Then this won't be necessary since the functions will have the appropriate attribute.
Feb 4 2019
Feb 1 2019
otherwise LGTM too
It's always been possible because clang and LLVM have always just been in different trunks in the same SVN repo. AFAIK it's never been disallowed but people rarely do it.
Jan 31 2019
LGTM. Can you put a little more detail in the commit message (e.g. something like what you wrote in the comment) to make it easier to see in a log (without having to look at the code or the PR)?
Jan 30 2019
Just noticed, but the discussion should maybe have gone into a comment on Phab rather than the commit message itself...
yeah, seems fine. we can always change our mind in the future even in the absence of a TODO :)
Jan 29 2019
Jan 28 2019
Jan 25 2019
overall it looks good; it's nice that this version is simpler.
Jan 24 2019
haven't quite gotten through it but here's what I have so far. Looking pretty good overall though.
LGTM; don't forget to include all the context in the future