- User Since
- Oct 15 2012, 2:12 PM (431 w, 9 h)
I'd like to avoid the bool completely if possible. Passing the register
might work as well if you wanted to do that, but I think it needs to be
clear API wise why and how you're doing computation where you are.
Sorry to point this out now, but taking a bool in/out pointer is some
fairly awkward API. Can we do something else here?
Fri, Jan 15
Can you provide a little more detail on the motivation here? Thanks!
I'm not quite sure I understand. What's the issue between xcoff and emission here? Does it not support subtraction? It's been a while and I can't recall.
Wed, Jan 13
If you changed the existing patch it won't be clear that this is something that needs review. Can you make a new review with the bug fix?
Tue, Jan 5
Dec 4 2020
Dec 1 2020
Thanks for the explanation, lgtm.
I think Florian's diagnosis is worth looking into for sure - and probably
is the right solution. For the regressions I had they could largely be
explained by the branch predictor on a micro level, I didn't see some of
the large scale LTO level regressions in mine.
Nov 20 2020
Why is this useful rather than having targets set the address space used in the backend explicitly? i.e. how is what amdgpu was doing up to this point not ok? It's not really clear here.
Nov 19 2020
Nov 16 2020
I think this is reasonable, if neither George or Peter say anything I think this looks ok to me.
Nov 13 2020
Also probably a good idea :)
I'd just seen that too. Thanks!
Nov 7 2020
This looks fine to me for now. I really think it's too wordy and too elaborate and could be about 1/10th as long.
Nov 5 2020
For the future you can just commit this sort of thing. Though I admit to being curious about the lint error :)
Oct 29 2020
Let's go ahead and unblock you, but getting a lot of this refactored would be great if you can. I think it's hitting the limits of the original design. :)
Oct 28 2020
I'll take a look tomorrow, sorry for the delay.
I'm going to accept this as it codifies a lot of existing intent behind how the community works. This was also fairly universally accepted 4 years ago in the original discussion and nothing has changed.
Oct 24 2020
Seems reasonable to me.
Oct 21 2020
I really do think this needs to be under object. It can be separate in there as lib/Object/Copy if you want, but I don't think it should be a parallel directory. Hopefully the updates for the move aren't overly onerous here.
Oct 14 2020
Awesome. Thanks for the update :)
Oct 13 2020
Sorry, didn't see this last time. Couple of inline comments, mostly nits.
Mark as requesting changes :)
Oct 12 2020
Oh, you're waiting on me. Uh, I'll get to this tomorrow. :)
Oct 9 2020
I like the direction this is going, I'll take more of a deep look soon, but wanted to ask: "should this be in Object rather than a separate library?" When I'd originally asked for this to be split into its own library I'd thought that it would get added into libobject.
Oct 8 2020
Arguably since this is going to be very cpu dependent perhaps it should be
part of TTI?
Oct 2 2020
Drive by as it's years since I've dealt with any of this:
Sep 18 2020
Sep 17 2020
Sep 16 2020
Sep 2 2020
I don't think your statement matches what I was seeing. I may have
constructed the testcase poorly, but there were differences being shown.
Also keep in mind that your clang run is testing what I've already fixed. :)
Sep 1 2020
I think this is the right thing to do here. In addition, we should not cherry pick this to any release branch so we have the opportunity to get it more fully evaluated by distros/vendors/etc over the next months.
Aug 29 2020
Aug 28 2020
Aug 26 2020
Aug 24 2020
The code has a pragma to turn on loop unrolling. I actually did want to check with optnone.
Aug 23 2020
LGTM from maintainability etc. Haven't really checked more so might want to wait for others.
Aug 21 2020
Aug 20 2020
Aug 19 2020
One more follow up here:
So, we're seeing several significant (20-30%) regressions due to this in various different library benchmarks. Usually around things that are compression/decompression loops, but also other places.
I feel pretty OK approving this.
Aug 18 2020
This would be because at that point the default cpu was that and it
probably had something to do with fallbacks.
Aug 17 2020
Aug 11 2020
Aug 10 2020
I'm also not sure this is passing?
Aug 9 2020
Aug 6 2020
I think that we should revert the original patch for now since we have a
known miscompile and a chain of fixes are required to fix it.
*thumbs up on ok for release*
This looks good to me. Thanks for seeing this through Ray!
Aug 5 2020
Aug 4 2020
Ow. Can revert and reapply after we fix the caching problem perhaps?
Could maybe add an assert along with the patch as well as an assert only
Jul 28 2020
I think I'd find the conditionals easier to "read" if they were positive rather than negative? It'd help if the comments spelled out the conditions a little more as well.
Jul 27 2020
This could probably use some tests. If no current upstream port uses the feature though it makes it hard to accept. :(
Jul 26 2020
I'd really like all of the cpu specific bits here to not reside in the objdump binary. Ideally it should be in include/ and lib/ somewhere.