- User Since
- Dec 4 2012, 1:14 PM (345 w, 2 d)
Nov 11 2016
Jun 29 2016
The patch LGTM. One question: Did you check for the reverse situation? Can we have a swap that is fed by more than one instruction, where one is not a load, or is that already handled in the code? I realize this may not come up in practice if we don't have a machine sinking pass.
May 2 2016
This looks fine to me. A couple of inline comments. I'll let someone else give final approval.
Apr 15 2016
Thanks. This LGTM!
Mar 30 2016
This patch LGTM. I was responsible for the original code here. Whatever problem I thought I was solving here appears to no longer be an issue, so let's go forward with this.
Feb 3 2016
Yes, that's the correct behavior, as can be seen at https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01937.html. Note that GCC produces a warning if the wrong operand type is used, but I don't see that as a necessity for something that's deprecated anyway. This patch LGTM.
Dec 14 2015
Dec 9 2015
Dec 8 2015
Committed as r255067.
Committed as r255059. Thanks!
Committed as r255057.
Nov 12 2015
This version adds the correct OPD implementation per Uli's suggestions. The only change beyond that is to make .L._setjmp and .L.__sigsetjmp unconditional symbols, so they can be used in the .size calculation for their respective functions.
Nov 11 2015
Oh, of course! Thanks, Uli. I totally forgot about having to declare the OPD. And it would be better if I were using _CALL_ELF rather than LITTLE_ENDIAN for the ABI distinction. I'll update the patch and try to solve my build problems to test it.
Actually, I managed to dig up a BE Power8 machine and run the tests, and although I have failures due to the setjmp handling, most of the tests pass for me. What sort of BE machine are you using? Is it a Power7?
Nov 10 2015
Simone, thanks for confirming. I've discussed this with one of our ABI experts. For BE, we are going to have to do something pretty ugly, I'm afraid. We'll likely need a trampoline to a separate function so that the linker will create an OPD from which we can load the TOC. But we'll still need to keep the link register clean along that path, which could be a little tricky.
Nov 9 2015
OK, thank you. I am wondering if this is a glibc "feature" when initializing a thread for Power. For LE, I noticed that it was not setting up the TOC for a call to setjmp from init_thread, which is technically within its rights only if setjmp cannot be overridden. Hence I added code to materialize the TOC from the .TOC. symbol. For BE, the TOC must be initialized by placing it in the OPD. If the OPD TOC slot contains garbage, which would be true if init_thread didn't set it up, then we have no way to materialize the TOC on BE systems. If this turns out to be the case, we would either need to disable TSAN for BE (not ideal), or try to figure out when setjmp is being called in this heinous way.
Nov 8 2015
Nov 6 2015
Nov 5 2015
I forgot to mention that I now have separate ways of materializing the TOC pointer for BE and LE. For BE, the TOC can be materialized from an offset of 8 from the beginning of the OPD, where the OPD is equivalent to the function address. For LE, all supported binutils define the .TOC. symbol, so that method can be used instead.
All comments should be addressed here. I have dropped the test case that still needs a little investigation. Again, this patch requires Simone's patches as a prerequisite. I had to do a little massaging of his compiler-rt patch to bring it up to current trunk.
Nov 3 2015
Thanks, Kyle. This LGTM. That's a much better interface to use (and one that didn't exist when this code was first written). Thanks for taking care of this!
Nov 2 2015
Updated to address Hal's comments. Thanks, Hal!
Sorry it took me a while to get back to this. I'll make all suggested fixes except for the ToErase one, as noted.
Oct 30 2015
Fixes a wrong-code problem.
Fixes an issue where a read of the CTR register is missed because it's hidden in an intrinsic.
Fixes a wrong-code issue.
Fixes a wrong-code issue.
Addresses a serious problem on pre-P8 hardware.
Fixes two not uncommon problems with condition bits.
Important bug fix for thread-local storage on Power.
Oct 27 2015
Oct 26 2015
Oct 22 2015
Oct 21 2015
Oct 16 2015
I should clarify that this is a bug that's independent of your work, so I think it should be treated separately.
Oct 14 2015
Sep 29 2015
Go to it!
Sep 28 2015
Sorry, forgot to accept.
Sorry, forgot to accept.
LGTM! Thanks for folding in the VSX compares to the existing infrastructure, and renaming appropriately.
Other than the FIXME removal, this LGTM! Very good and careful work. Thanks for taking care of this!
Sep 21 2015
Otherwise, this LGTM. Thanks for addressing all my concerns!
Sep 15 2015
Sep 14 2015
Sep 4 2015
Sep 3 2015
First, congrats on sorting this out; bootstrap issues are always a pain. I have one minor inline comment. Also, can you add a test case for this situation?
Aug 24 2015
Aug 18 2015
Jul 29 2015
A few issues and questions...
Jul 28 2015
Committed as r243467.
Jul 27 2015
Revised patch posted at http://reviews.llvm.org/D11552, which replaces this review.
With these changes, the ASAN test suite passes on powerpc64le.
Result with those changes:
Thus I can fix this by making the following changes to Bill Seurer's patch:
- Remove changes involving stack popping;
- Remove the kFastUnwindAddPC change; and
- Change calculation of pc1 as follows:
OK, I've been looking at a small example. I've taken the portion of Bill Seurer's patch that modifies the offset to the return address in the stack frame, but omitted the part that pops extra stack frames, so that I can get a look at why we get a bogus extra frame.
Jul 24 2015
Jul 21 2015
Jul 20 2015
Jul 16 2015
Jul 15 2015
Jul 14 2015
Please double-check the divide code generation. So long as we aren't scalarizing the code, this LGTM.
Jul 13 2015
Jul 9 2015
I would also like to see a test case or two for the confusing -mno-vsx -mpower8-vector stuff...
LGTM with additional commentary to explain what's going on. :) Thanks.