- User Since
- Nov 12 2014, 1:58 PM (123 w, 6 d)
Sun, Mar 26
Fri, Mar 24
Can you add a testcase to show the difference?
Thu, Mar 23
Oh, sure. Sounds sensible (modulo nits).
I mean, all external functions, not just libcalls, e.g.
declare noalias i8* @blahgoo(i32) [...] call void @blahgoo(i8* %tmp)
... unless they're already preserved, in which case, feel free to ignore this comment.
You want to do this for all the external functions, maybe?
Wed, Mar 22
lg, thanks for the fix.
I'll run this through the testsuite and commit on your behalf soon.
Tue, Mar 21
I thought about the same problem Mehdi mentioned, but I have to say it's not Filipe's fault. In fact, the root cause is lack of proper integration testing in LLVM (GCC has infrastructure for that, FWIW).
No other comments from me, I'll be happy if @chandlerc can take a look
Some initial comments inline. Meta comment, I think this patch can be split in multiple ones, (e.g. you can have yaml2asm separately).
That would make the review easier/quicker, and so would be my life as reviewer.
Mon, Mar 20
Sure, why not. Feel free to commit debug cleanups without pre-commit review.
Sun, Mar 19
Sat, Mar 18
Can you add a testcase?
Fri, Mar 17
I actually reviewed/LGTM'ed this one days ago but phab didn't send the message. Oh, well.
Thu, Mar 16
Other than the comments, this looks good
Wed, Mar 15
Reid (@rnk) pointed out this comes up during Chromium build so please make sure to verify the output is still reasonable with this patch (if I recall correctly libtool emits something similar (but it's more verbose) and the Chromium folks have to filter it out).
Tue, Mar 14
Sun, Mar 12
LGTM. I personally like your plan of removing the cache altogether supported by benchmarks.
Sat, Mar 11
I agree with what Simon said. Some comments inline.
You fixed all the style comments, so thanks. But you glossed over the design questions I asked. In particular, before getting performance numbers, there are two questions unanswered:
- Why can't you do this in the middle-end? I assume there's a valid reason, but I'll be happy if you can actually elaborate.
- Granted we want this in the backend, why are you putting the pass at that specific point in the pipeline?
Fri, Mar 10
Thu, Mar 9
Wed, Mar 8
Wei, any progress on this? We should either commit a workaround or revert the patchset to unblock folks.
Tue, Mar 7
Mon, Mar 6
In other words, if you find other cases where this help, fair enough, but I'm reluctant to introduce new code in ArgList with a single consumer for cosmetic reasons only.
I'm not entirely sure it's worth the complexity. This just allows the lld driver to return early.
Sun, Mar 5
ehm, what's your use case for iterating lld arguments in reverse order?
This looks fine. It would be nice if you could keep the doxygen comment.
LGTM, with the inline comment addressed. Thanks!
Is it possible to address this at IR level? Anyway, comments inline.
Wed, Mar 1
The FreeBSD related bits are correct. The whole chain of #ifdef is a little nasty, but I'm not sure there's an easy way to avoid it.
Reducing malloc() traffic is generally a good thing, so is this patch. It's particularly slow on some systems (Windows), and FWIW, not great also on Linux with the default allocator (I ended up replacing my malloc() with tcmalloc everywhere, but that's a story for another day). That said, I'm curious if you're actually able to measure any compile time reduction with this change on the cases you plan to switch.
I really apologize for the delay in providing you the patch. I'm at GDC this week and that limits my ability to work on LLVM upstream things. I will provide the patch to you next week, and if I don't because I forget, feel free to ping me again.
I don't think this is bad, but can you please upload the other change to show up how you want to use it?
The logic looks good. I'll let Rui review the style.
Tue, Feb 28
The Domtree changes look good to me but I can't comment on StructurizeCFG
Mon, Feb 27
@qcolombet what do you think?
Feb 25 2017
Also, I think the test from the PR (after some polishing, maybe) is good to add. If somebody removes these lines at least all (or a subset of) the bots will timeout and we'll notice the regression.
I actually forgot to recommit this one :(
The issue with the original patch was that I folded the call inside the assert so ConstantFoldTerminator() was compiled away in !DEBUG mode.
Oh, well. Your reasoning is correct (actually, was my original reasoning when I committed the patch).
Feb 24 2017
Probably this is OK as stopgap solution but I'm generally very wary of adding cutoffs to the code as they add technical debt etc..
Feb 23 2017
Feb 22 2017
Agree with Eli. LGTM.
Feb 21 2017
Feb 20 2017
Feb 19 2017
Feb 18 2017
Oh, just realized I commented while you're updating. Oh well.
The code logic looks fine. Some minor comments inline.
LGTM! (after some minor comments inline have been addressed).
I used this (yesterday) to track down an issue internally, so very happy about this feature already.
I'd commit the PredicateInfo bits separately (it would be awesome if you can actually a test for that).
Feb 17 2017
What's the compile time savings you're seeing from not computing these analyses unnecessarily?
I think a test for infinite loops (the one attached to the PR or/and slight modification of them) are fine (at least for me)
The approach looks good to me, if you add a testcase and revert the unrelated changes I'll give it another look (possibly today)
When I saw this GCC feature I was really jealous =) Thanks for working on this, reviewing it now.
You can probably on and commit a subset of this. I'm in doubt about the first chunk so I asked Danny/George to take a look.