- User Since
- Oct 15 2012, 2:12 PM (274 w, 1 d)
Fri, Jan 12
Also the register saves strike me as a somewhat odd optimization - you could also have just thrown this into SAVE_REGISTERS I think?
A bit late, but I have some concerns here.
Aha. I missed something. :)
Thu, Jan 11
I think instead you want to add an alias in PPC similar to this:
Tue, Jan 9
Mon, Jan 8
I think you're missing that right now it's x86 only yes? :)
Fri, Jan 5
One inline comment and nit, but then I'm ok with this. Should elaborate about why we're doing this in the commit message.
Just to verify, this is effectively hand scheduling the prologue and epilogue?
Thu, Jan 4
LGTM of course.
Wed, Jan 3
Couple of inline comments, otherwise I'm pretty happy. I'd wait for an ack by Richard for this though.
Tue, Jan 2
Thu, Dec 21
Mon, Dec 18
Dec 11 2017
I think one of my concerns here is that this also shows up on haswell with > 128-bit vectors. How would you want to fit that in here? A "Prefer128SIMD"?
One inline comment then LGTM.
Dec 8 2017
Can you recombine this fix in with the patch that was reverted in r319218?
Dec 7 2017
Dec 6 2017
Dec 5 2017
This needs careful review. I'll take a look as soon as I can.
Nov 30 2017
Nov 29 2017
This definitely needs numbers - across multiple platforms. Both performance and size of resultant binaries.
FWIW that's typically how we do things.
I'd really like to see this solved in a more generic way, but I think that's a bit far afield for a single backend so OK.
Nov 28 2017
How are we generating these isels if they're basically nops?
Nov 27 2017
Nov 20 2017
No strong opinion. I think I like that one more though.
This seems strictly more difficult to keep under control? Though I guess the assert helps.
Nov 15 2017
So, what's going on with the CurrCycle counter as we're going through? If we were, instead, to fix that would we still need this patch?
Nov 14 2017
All at once with an update tool is my preference. :)
I've made a couple of comments on API naming and deprecating inline.
Nov 13 2017
Can you break out some of this into "NFC statistics updates" and "the rest of the patch"? It'll make it much easier to review.
Couple of comments:
Nov 9 2017
Seems to be a step in the right direction anyhow.
I really appreciate you doing this work. This has been something I've wanted for some time :)
Nov 1 2017
Oct 27 2017
That looks pretty usable for someone as bad at C++ as I am so I'm in favor.
Oct 26 2017
Oct 24 2017
Looks much better to me. Let sfertile give a final ACK since he's been in here too, but I'm happy. One inline comment :)
Oct 23 2017
This should be fine for now. Thanks!
Oct 20 2017
No objections here.
Oct 18 2017
I guess number of instructions is rather hard coded in. I'd definitely like to see this across other targets, but it works for now.
Oct 17 2017
First pass on review.
Oct 16 2017
Please update the comments in both the code and the testcase to express what we're looking for and why we're looking for it. Right now if something breaks no one will know why.
Oct 13 2017
Adding in more reviewers. In general I'm fine here, but since they've been in this area much more I'd like to hear from them.
Given you were the last one in this code it seems reasonable to let you go for it :)
Oct 12 2017
Oct 10 2017
Code generation looks a lot more reasonable. Can you commit the change to test/CodeGen/PowerPC/sjlj.ll separately?
Oct 9 2017
Other than being relatively certain some of your lines are > 80 columns this is fine :)
Seems that having a value parameter for MII in all of these targets is a little less than desirable. Also, perhaps you could just add it to the base class if you're going to use it everywhere by default?
In general a fan of this FWIW:
Oct 5 2017
This generally works for me modulo the things that Hal has mentioned. You'll probably want to add Richard to the review list for the Sema bits as well.
One high level comment that I added inline as well:
One question here: Where are the sign and zero extensions coming from? I want to make sure we're not producing extra ones where we could avoid it.
Oct 3 2017
Obviously I haven't read the entire patch. I'm in favor of this change though and so will approve it :)
Oct 2 2017
Sorry for the delay, go ahead.