- User Since
- Apr 27 2015, 11:17 AM (151 w, 2 d)
Wed, Mar 14
Tue, Mar 13
Fix binutils version in FIXME
Workarounds for old plugin-api.h
I had to revert as the new api is not available:
LGTM, but what about the regular LTO case (i.e. we set this directly on the GV in LTO.cpp, instead of on the summary and then propagating in the backends here for ThinLTO)?
Mon, Mar 12
LGTM with comment suggestion below
Without this patch, importing !callees metadata would only add promoted declarations of @f1 and @f2 to the bar.o, but still the optimizer will assume that the function is available and perform the promotion.
Sorry for the lateness of the review! Comments/questions below.
Thu, Mar 8
LGTM with some minor comments below.
LGTM with fixes noted below.
Wed, Mar 7
Tue, Mar 6
Sorry for the delay. I don't feel super confident reviewing these change. I looked back at who touched the ComputeLoadResult code last (besides pcc who simply extracted it out of GlobalOpt into Utils), and it was Chris Lattner in 2005! So I am not completely sure who to add to as a reviewer at this point.
Poking around LLVM though, I see that there is some very similar handling in llvm::ConstantFoldLoadFromConstPtr (ConstantFolding.cpp). In particular, the handling of P as a GlobalVariable or GEP appears identical to what is already in Evaluator::ComputeLoadResult. It already has handling for bitcasts, although that handling appears quite a bit more complex that what you added to ComputeLoadResult. I'm wondering whether ComputeLoadResult can be refactored to just call ConstantFoldLoadFromConstPtr, or whether there is a reason it needs to remain distinct (and why the bitcast handling added here is simpler).
Mon, Mar 5
Sorry for the delay, I was OOO for awhile and am still catching up.
Thu, Feb 22
Thanks for the clarification - I focused on the wrong thing given the change to Filenames/IsTemporary.
Wed, Feb 21
Can you clarify the motivation for this change - I don't see what is significantly different. The exit(0) for the index-only case is still in the same location and we just end up with a different data structure (Files) on the stack. AFAICT this just trades 2 vectors for a vector of tuples.
Tue, Feb 20
Feb 16 2018
Feb 15 2018
Feb 14 2018
LGTM thanks for the cleanup
LGTM with a request for a comment below.
Feb 12 2018
+rafael who is a bit more familiar with this part of the IR linker.
Feb 11 2018
Thanks! Looks like the test accidentally got left out of the commit, so please commit that as a follow-on.
Feb 9 2018
Nice! Glad to hear there are some measurable improvements. A few comments/suggestions below.
LGTM although I have a suggestion on where to move the new code.
Wondering whether there is a better way of proactively catching these issues - I assume it is currently ultimately detected through either linker failures or even worse, runtime failures. The only fully automated idea I have come up with is to have a debugging mode where we would force a native object to be produced even for cache hits, and would assert that the new and old objects are identical. Since that is expensive, the question would be how to run that broadly enough to catch bugs like this quickly, before it reaches the wild. Since the LLVM revision is used in the key, it would have to be using the same clang to build something large enough but at either different source revisions or with different enough options.
Feb 7 2018
I'd like @pcc to comment. I noticed there are a number of other places in this file where we will still be calling replaceAllUsesWith on a Function. How do we know which should use which method?
Feb 6 2018
Empty ThinLTOIndexFile signals that we don't need this module during
LGTM with the below changes. Thanks for the cleanup!
Feb 5 2018
Update to reflect changes to drop dead variables and aliases
ping - @pcc any objections?
This looks ok to me, assuming it produces the empty output file as I expect it should. Please wait for pcc to take a look as well.
Feb 2 2018
Feb 1 2018
LGTM with some comment requests below.
Jan 31 2018
Jan 30 2018
It looks ok to me, but I am not sure how ld64 is using this - are they also setting to 0 by default expecting default behavior?
Update per comments
Ping. It turns out this fixes https://bugs.llvm.org/show_bug.cgi?id=36089 (I tested the new version that uses the ValueMapper). @pcc: let me know if you prefer the ValueMapper approach (see my previous comments), and I will update the patch officially.