In D26564#593352, @hfinkel wrote:

@wschmidt and other IBM folks, w.r.t. this and D26566, can you please comment on what GCC does here? Does GCC use -fPIC by default for PPC64 (literally or in effect)?

In D21802#470163, @kbarton wrote:

In D21802#469887, @wschmidt wrote:

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.

This is already handled in the code. If the register defined by a swapping load is used in something other than a swapping load/swapping store, then the web is rejected. Unless there is another case outside of this that I'm not seeing....

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.

This looks fine to me. A couple of inline comments. I'll let someone else give final approval.

Thanks. This LGTM!

In D19133#402129, @nemanjai wrote:

Just one inline comment (actually a question for my understanding) and pending a test case, this LGTM. Perhaps @wschmidt would like to take a look as well.

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.

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.

r255507

Committed as r255067.

Committed as r255059. Thanks!

Committed as r255057.

In D12841#304084, @simoatze wrote:

Addressed Dmitry's comment, patch ready to land.

Bill, will you take care of the commit?

Thanks!
Simone

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.

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.

In D13729#287039, @wschmidt wrote:

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?

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?

r252651, thanks!

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.

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.

Ping

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.

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!

In D13729#279937, @dvyukov wrote:

Bill, you did not upload a new version. Waiting.

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.

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.

Ping...

In D12596#268887, @hfinkel wrote:

In D12596#268816, @wschmidt wrote:

In D12596#268715, @nemanjai wrote:

So the crux of the problem is that the load being replaced has a user of its chain (in this case, a store that nullifies the unique_ptr in the source). Because we introduced the LXVDSX, the use of the chain in the store was not updated and it still used the chain from the load (which goes away). The store was then free to move up (before the LXVDSX) and we end up with a load and splat of a null. I do not see a way to chain the store to this target specific node.

So, you're going to have to ensure that the DAG nodes for LXVDSX have chains on them. This is an oversight in the way things are implemented today. See what's done for LXVD2X:

def PPClxvd2x  : SDNode<"PPCISD::LXVD2X", SDT_PPClxvd2x,
                        [SDNPHasChain, SDNPMayLoad]>;

So you'll need to add LXVDSX to the PPCISD enumeration in PPCISelLowering.h, add an entry like the above in PPCInstrVSX.td, and make sure we expand to that node type in the DAGtoDAG code. Then you'll have a chain that you can manipulate.

You don't need to add special ISD nodes to do instruction selection in DAGToDAG (only in ISelLowering). LXVDSX is already tagged as mayLoad, and so is already assumed to carry a chain operand. In DAGToDAG we directly generate machine-instruction SDAG nodes. You just need to make sure that chain users are appropriately updated before returning.

I should clarify that this is a bug that's independent of your work, so I think it should be treated separately.

In D12596#268715, @nemanjai wrote:

So the crux of the problem is that the load being replaced has a user of its chain (in this case, a store that nullifies the unique_ptr in the source). Because we introduced the LXVDSX, the use of the chain in the store was not updated and it still used the chain from the load (which goes away). The store was then free to move up (before the LXVDSX) and we end up with a load and splat of a null. I do not see a way to chain the store to this target specific node.

Go to it!

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!

Otherwise, this LGTM. Thanks for addressing all my concerns!

LGTM! Thanks.

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?

A few issues and questions...

Committed as r243467.

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:

In D9259#213181, @samsonov wrote:

In D9259#213127, @wschmidt wrote:

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.

In the example, we produce this:

freed by thread T0 here:

#0 0x1010d988 in GetStackTraceWithPcBpAndContext /home/wschmidt/llvm/llvm-test2/projects/compiler-rt/lib/asan/asan_stack.h:50
#1 0x1010d988 in operator delete(void*) /home/wschmidt/llvm/llvm-test2/projects/compiler-rt/lib/asan/asan_new_delete.cc:94

^^
Why do we output two identical (0x1010d988) addresses here, if you refer to them as 0x1010d98c and 0x1010d910 later?
We do call StackTrace::GetPreviousInstructionPC() when we symbolize addresses, but why isn't second address turned into 0x1010d99c?

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.

Please double-check the divide code generation. So long as we aren't scalarizing the code, this LGTM.

LGTM.

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.