- User Since
- Oct 15 2012, 2:12 PM (257 w, 14 h)
LGTM. Let Nemanja give the final approval.
This is great with me.
Tue, Sep 12
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.
Fri, Sep 8
Couple inline comments, first should be handled, otherwise looks OK to me.
Thu, Sep 7
Tue, Sep 5
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.
Wed, Aug 30
Tue, Aug 29
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.
Mon, Aug 28
Fine with me.
Sun, Aug 27
One inline comment, but go ahead and commit after fixing that up.
Mon, Aug 21
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.
Jul 19 2017
<insert comment about offline discussion about the two lists unfortunately not syncing>
Jul 18 2017
LGTM pending dependency approval
Can you update the code in CGBuiltin to use this as part of this patch? :)
From a high level it sounds fine. You'll probably want someone else that's hacked on cmake to give an ack though. :)
FWIW the duplication in CGCall.cpp of the enum set is painful if you can come up with anything else it'd be awesome. I don't have any good ideas, just a fond wish :)
From my perspective here once you and Erich get some agreement on the checking between your two bits I'm fine :)
Adding Erich Keane here on this since he's working on something similar for the target attribute.
If you wouldn't mind I'd like to see this separated out a bit -
a) I think we can make the attribute info a struct rather than a pair as a simple change first
b) setCPU split
c) let's see where we are after that?
Jul 14 2017
This sort of thing seems obvious in the future :)
Jul 13 2017
Jul 12 2017
Not quite what I meant, but if you want to ensure that this crashes the compiler rather than emitting bad code then this is fine. Can you also add a TODO with "this should return an error" or some such?
Not a huge fan, but also don't have any better ideas for now.
Jul 11 2017
Jul 10 2017
Perhaps an explanatory comment? :)
Jul 9 2017
Jul 6 2017
Jul 5 2017
Jun 30 2017
This is how you should do it. Though I'm not sure it isn't just better to check for "processor >= power9", but *shrug* for now.
Jun 29 2017
I've committed this for you as:
I've gone ahead and committed something very similar thusly:
Committing to https://llvm.org/svn/llvm-project/llvm/trunk ...
Sorry, when I say "One inline comment otherwise LGTM" feel free to commit after fixing :)
Jun 28 2017
Can you give me an example here of what you're trying to accomplish? I'm not sure I understand.
Jun 26 2017
Reid is still the best reviewer for this.
Jun 22 2017
Jun 21 2017
This is OK once the dependent revision is approved.