Page MenuHomePhabricator

[PowerPC] Do not emit displacements for DQ-Form instructions that aren't multiples of 16
ClosedPublic

Authored by nemanjai on Jul 5 2017, 4:43 AM.

Details

Summary

The PowerISA 3.0 defines some instructions (such as LXV and STXV) that take a displacement as a quad-word offset (i.e. the effective address is calculated by shifting the value left by 4). As a convenience and consistency with DS-Form instructions, the assembler takes a byte offset. So it is meaningless for a byte offset to not be a multiple of 16.
The assembler already complains when assembling these instructions with an incorrect displacement, we just need to make sure we don't emit them this way.

This patch also fixes https://bugs.llvm.org/show_bug.cgi?id=33671.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Jul 5 2017, 4:43 AM
nemanjai added inline comments.Jul 5 2017, 4:46 AM
lib/Target/PowerPC/PPCInstrInfo.td
408 ↗(On Diff #105250)

If this solution is the way we want to proceed, perhaps it would be good to use the same approach for DS-Form instructions. These ultimately don't need to be aligned, they just can't have a displacement that isn't a multiple of 4.

hfinkel added inline comments.Jul 5 2017, 7:35 AM
lib/Target/PowerPC/PPCInstrInfo.td
408 ↗(On Diff #105250)

Why can't we just check cast<LoadSDNode>(N)->getAlignment() >= 16; like we do above?

nemanjai added inline comments.Jul 5 2017, 7:52 AM
lib/Target/PowerPC/PPCInstrInfo.td
408 ↗(On Diff #105250)

But we are not actually interested in alignment. These instructions do not have an alignment requirement. The only issue is that the DQ field encodes a quad-word offset (and what we're working with here is a byte offset).

I can certainly see that the names are misleading and would be happy to change them. The only reason I didn't is that it seems like we might want to have some consistency with the above fragments.

The test case I added illustrates the intent. It comes from this:

void test1(int *arr, int *arrTo) {
  vector int *ptr = (vector int *)(&arrTo[4]);
  *ptr = *(vector int*)(&arr[4]);
}
void test2(int *arr, int *arrTo) {
  vector int *ptr = (vector int *)(&arrTo[1]);
  *ptr = *(vector int*)(&arr[2]);
}

The test1 function can use the DQ-Form instructions. The test2 function cannot. And the alignment on the loads/stores is the same.

hfinkel added inline comments.Jul 6 2017, 6:25 PM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
3022 ↗(On Diff #105250)

Please write this as:

SDValue AddrOp = cast<MemSDNode>(N)->getBasePtr();

lib/Target/PowerPC/PPCInstrInfo.td
408 ↗(On Diff #105250)

I can certainly see that the names are misleading and would be happy to change them.

Yes, please do.

nemanjai updated this revision to Diff 105651.Jul 7 2017, 9:32 AM

As it turns out, I was wrong in my assessment of Hal's comment. Although the instruction doesn't require any special alignment, allowing it to be used with weaker alignment allows other passes to modify the offset after ISEL. So the fact that we verify that Offset % 16 during ISEL isn't sufficient.
Furthermore, when we eliminate the FrameIndex the code does not have any awareness of instructions that need an immediate that is a multiple of 16.
I've made the following updates:

  • Don't emit LXV/STXV for unaligned loads/stores (use the X-Forms for that)
  • Teach FI elimination that some instructions need immediates that are multiples of 16
  • Fix the test cases that changed behaviour
  • Assert if we somehow end up with an instruction that needs an immediate as multiple of 16 but has an incorrect immediate
  • Specify alignment to the function that generates the displacements during ISEL
  • Add missing DS-Form instructions to the list of instructions that need a multiple-of-4 immediate
  • Fix loads/stores that were using incorrect addressing
  • Divide up the loads/stores into those that can handle unaligned addresses and those that can't
jtony added inline comments.Jul 7 2017, 9:59 AM
lib/Target/PowerPC/PPCISelLowering.cpp
2143 ↗(On Diff #105651)

Minor nit, the variable "imm" should be Imm, we can fix it in this patch.

2167 ↗(On Diff #105651)

Same here.

2194 ↗(On Diff #105651)

This one is already correct.

lib/Target/PowerPC/PPCRegisterInfo.cpp
895 ↗(On Diff #105651)

noImmForm --> NoImmForm

test/CodeGen/PowerPC/PR33671.ll
1 ↗(On Diff #105651)

These Attrs are unnecessary, right?

hfinkel edited edge metadata.Jul 11 2017, 8:00 AM

Although the instruction doesn't require any special alignment, allowing it to be used with weaker alignment allows other passes to modify the offset after ISEL.

For what other passes is this true (aside from places dealing with frame indices, which it seems like you're fixing regardless)?

lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp
274 ↗(On Diff #105651)

If you use the builtin assembler directly, can a user hit this assert? If so, we should put an actual diagnostic somewhere.

Although the instruction doesn't require any special alignment, allowing it to be used with weaker alignment allows other passes to modify the offset after ISEL.

For what other passes is this true (aside from places dealing with frame indices, which it seems like you're fixing regardless)?

I actually don't know that. I thought I had seen that before I fixed all the FrameIndex stuff, but looking back on it - it was also FI related. Do you think I should go back to the original way of testing for the displacement operand, but keep the FI fixes?

lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp
274 ↗(On Diff #105651)

No, the internal assembler already handles the diagnostic due to the definition of the instruction due to the definition of the operand being correct:
isS16ImmX16 is already defined to test for whether the operand is a multiple of 16.

Although the instruction doesn't require any special alignment, allowing it to be used with weaker alignment allows other passes to modify the offset after ISEL.

For what other passes is this true (aside from places dealing with frame indices, which it seems like you're fixing regardless)?

I actually don't know that. I thought I had seen that before I fixed all the FrameIndex stuff, but looking back on it - it was also FI related. Do you think I should go back to the original way of testing for the displacement operand, but keep the FI fixes?

Yes. (although you should still change the name of the tablegen predicates to something that does not imply that we're checking alignments).

nemanjai updated this revision to Diff 106336.Jul 12 2017, 4:15 PM

Kept the fixes to the FrameIndex issues but reverted the predicate for selecting the DQ-Form instructions to use the displacement being a multiple of 16.

As it turns out, the problem reported in the bug is much more serious and prevalent - affecting P9 code generation for most applications. This is really something we should get into the 5.0 release as we don't want to ship a release with this bug. We branch the release next Wednesday so I'm really hoping we can get this reviewed and committed by Friday of this week to give it a couple of days in trunk before we branch.

@hfinkel @echristo @kbarton I know I'm asking a lot, but would you please give this patch your prompt attention due to the prevalence of the problem and the imminence of the release branch.

hfinkel accepted this revision.Jul 13 2017, 6:32 AM

LGTM

This revision is now accepted and ready to land.Jul 13 2017, 6:32 AM
This revision was automatically updated to reflect the committed changes.