- User Since
- Oct 15 2012, 2:12 PM (414 w, 4 d)
Fri, Sep 18
Thu, Sep 17
Wed, Sep 16
Wed, Sep 2
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. :)
Tue, Sep 1
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.
Sat, Aug 29
Fri, Aug 28
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.
Jul 25 2020
Jul 23 2020
Jul 17 2020
(Adding Sterling as well)
I think this will be an OK workaround for now, go ahead and reply on the main thread as I think they'll want to set the default runtime library as part of the toolchain. I'm surprised at this behavior as well.
Jul 16 2020
Appears so, but ok.
Jul 15 2020
It's that even before the msan instrumentation the IR doesn't look correct - thus a miscompile.
We're starting to see miscompiles as we do more testing as well, just nothing smaller at the moment.
This seems like something that we should then revert until we know that instsimplify can be updated with a fix?
Did you want to just change the pentium4 tuning? Otherwise naming things is hard and this feels awkward, but I don't have any better ideas :)
Jul 14 2020
Jul 13 2020
Jul 10 2020
Jul 9 2020
I may need to comment it better, but in this case part of it is that it's designed to only have the forced loop unroller run on it.
Some inline nits. I see you've already committed and that's fine - I still don't think we should do it, but we can delete it again soon :)
So the tuning here for SCE is also a "does not support" or something else?
Jul 8 2020
In whose builds and why? What changes would you like? What compiler are you using? Can you provide any more information?
Jul 7 2020
If this works then awesome. :)
Jul 6 2020
Works for me :)