- User Since
- Oct 15 2012, 2:12 PM (265 w, 6 d)
Wed, Nov 15
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?
Tue, Nov 14
All at once with an update tool is my preference. :)
I've made a couple of comments on API naming and deprecating inline.
Mon, Nov 13
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:
Thu, Nov 9
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 :)
Wed, Nov 1
Fri, Oct 27
That looks pretty usable for someone as bad at C++ as I am so I'm in favor.
Thu, Oct 26
Tue, Oct 24
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 :)
Mon, Oct 23
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.
Sep 25 2017
Sep 21 2017
Seems like we might want to have block layout clean up this sort of branch rather than analyzeBranch since we could theoretically make the same change in every backend?
Performance numbers? SPEC at least :)
Much better. Thanks for the descriptions!
Other than the readability issue, sgtm.
I mean, I disagree with the general behavior, but I think I've lost that argument so if it's consistent go ahead with it :)
Adding Paul since he's looked at this stuff extensively.
Sep 20 2017
High level comment: The documentation on the processor in the model here is amazingly sparse (or rather non-existent). It'd be great if for every itinerary you describe the particular pipeline aspects it's covering and how it is associated to the overall CPU.
Sep 19 2017
Sep 18 2017
LGTM. Let Nemanja give the final approval.
This is great with me.
Sep 12 2017
Let's go ahead with this one for now. Sam's patch can go on after if we decide to go with a C API way of accessing this rather than just inline assembly.
Acking to clear the no bit.
Looks totally reasonable to me. Going through and cleaning up the last set of bool flags when you get a chance would be nice.
Sep 8 2017
Couple inline comments, first should be handled, otherwise looks OK to me.
Sep 7 2017
Sep 5 2017
The two I had were 0xf2666d and 0xf3666d that were broken with the earlier patch that I mentioned in my reversion email following up on the original.
Aug 30 2017
Aug 29 2017
Have the front end generate all of the correct target features for your stub then rather than adding them in the backend?
Which isn't necessarily going to be valid either - particularly in the face of LTO.
Not the way I'd have done this. What you probably need to do if adding a stub is propagate the features from the function you're cloning rather than constructing a new (and possibly incomplete) set of features from the target triple yourself.
Aug 28 2017
Fine with me.
Aug 27 2017
One inline comment, but go ahead and commit after fixing that up.
Aug 21 2017
Seems reasonable to add a comment as to what microarch features we're attempting to target as modern here.
Aug 10 2017
Aug 9 2017
Aug 4 2017
In general looks ok. Mind commenting the if(NeedLoad) areas as to what you're adding on and why? The symbol is (IMO) obvious, but the random bits less so.
Aug 3 2017
One inline comment, otherwise lgtm.
I'm ok with this.
Aug 2 2017
Jul 31 2017
Jul 26 2017
Tagging in Matthias here.
Jul 25 2017
Jul 21 2017
I'm going to say this ahead of time without looking into it "LGTM", but wait for ctopper (or someone else) to ack it for style etc since I'm unlikely to get to it any time shortly. :)
Jul 20 2017
So, what's the overall logical idea behind the need for this option? While I understand that you don't want people just doing this via the -mllvm set of options, but if you're just doing this to provide a temporary option rather than turning it off in the backend I'm not sure what the point is.