- User Since
- Jul 7 2012, 2:54 PM (336 w, 9 h)
Wed, Dec 12
Wed, Dec 5
Tue, Dec 4
One place that I'm not seeing an update reflected (but I may just misunderstand)....
(just marking that this is waiting on update from Fedor)
Please focus the discussion on the llvm-dev thread rather than here.
Marking as waiting update from Mehdi and adding Richard as a reviewer since this will need to deal with what our defined volatile semantics are for bitfields which seems like the kind of thing he would want to be very aware of...
I think this should be internal-driver-option and the text updated? I don't think these are necessarily experimental, they're internals of the compiler implementation, and not a supported interface for users. Unsure how to best explain this.
Most of the test case changes make sense to me.
Fri, Nov 30
Thu, Nov 29
Wed, Nov 28
While I like the update (keeping any potential changes to Optional and SmallVector separate seems good) I have a serious concern that this has the same fundamental problem as the prior mentioned ABI issue.
Sun, Nov 25
Mehdi pointed out that his primary concern is *volatile* access.
At least for TSan, this is undesirable -- we want a chance to observe these as potential data races.
Sat, Nov 24
Adding new support for a vector math library seems worth getting reviewed / approved by someone reasonably deeply familiar w/ our vectorization infra.
Fri, Nov 23
Thu, Nov 22
In an IRC discussion, some interesting points were raised about this, and I wanted to try to give a better naming pattern and more concrete outline. I'll do it by giving a pseudo output tree I could imagine:
I somewhat agree w/ Philip that this seems like too much complexity / machinery for what it does...
Wed, Nov 21
Tue, Nov 20
FWIW, I really like the direction you and Dave are pursuing. My random thoughts about how to make progress below:
Bunch of somewhat minor API cleanups that seem reasonable to do when touching these specific bits of the code...
As was mentioned in the original patch, this does still need tests. There are tests here, for example in test/Analysis/CostModel/.... You should be able to see some differences in costs, especially for architectures where the resulting cost computation differs significantly.
Mon, Nov 19
This patch was never sent to llvm-commits.
We're all still waiting on a fix here....
Fri, Nov 16
Nov 15 2018
Nov 14 2018
Really cool, and nice job with the first round of things. Very nice testing!
Reorganize the company listing and add UIUC as signed.
Should likely add release notes about this.
Makes sense to me generally. Some questions below really.
Nov 13 2018
Nov 12 2018
Sorry that this comment has gotten lost *twice*. I tried to write it over a week ago. =[[[[
I think this is looking pretty good as an initial cut. I'm still very interested in follow-ups to use a more precise heuristic of course, but I think those can be follow-ups.
Nov 10 2018
FWIW, this looks really good to me. I'd be fine with this going in as-is and any further improvements suggested by David as follow-ups. Feel free to submit with comments addressed.
Generally feel free to submit w/ comment and commit message updates.
I really do find the break with convention here somewhat deeply unfortunate. It makes parsing and recognizing triples reliably much harder IMO. This really should be 'hurd-gnu' or 'mach-gnu' (or however you want to spell the 'OS' here).
Nov 9 2018
Seems pretty reasonable to me. Pretty minor things below...
Nov 8 2018
I like you suggestion of writing FileCheck against print<loops> output.
FWIW, still LGTM and I think you can go ahead and submit.
This is the legacy PM, and specifically as it is implemented in codegen, so my opinions are few and far between... Not sure the benefits here are all that important, but maybe? Looping in folks w/ more backend feelings about this kind of thing.
LGTM, nit below.
Nov 7 2018
Honestly, this seems like a really reasonable approach in practice.
LGTM too (but always feel free to submit w/ Alina's review)
Could you not introduce all the separate test files?
Nov 2 2018
Nov 1 2018
Oct 31 2018
Curious how others feel, but I'd prefer to keep all the parsing in the pass builder rather than start calling back into the passes. I think that keeps the passes more minimal, and I don't think this kind of parsing should he so widespread as to need separation. If a few passes have custom logicbib pass builder that seems ok...
I think this is fine to go in, a minor suggestion inline. Rest can be follow-up.
Oct 30 2018
In addition to what Eric said, I just want to pull back up an old comment here:
Oct 26 2018
Awesome improvement, thanks!
Generally looks fine, two minor adjustments below and LGTM once the underlying cleanuppad change lands.
FWIW, after my suggestion I would not call this "NFC" as it should change the cost function of this routine at least....
While this is a neat way to handle this, I'm a bit worried about the effects of it...
Oct 23 2018
Sorry for delay...
Oct 22 2018
Minor comments on test case, otherwise this LGTM. Feel free to submit with a minimized test case.
I'm really just not sure how all this complexity is supposed to sort out. Maybe Chris has some thoughts here?
The intent of noinline in LLVM's IR is to block inlining, not all interprocedural optimizations. So I don't think this is actually a bug and don't think this patch should be applied.