Page MenuHomePhabricator

PowerPC/SPE: Fix load/store handling for SPE
Needs RevisionPublic

Authored by jhibbits on Nov 11 2018, 2:58 PM.



Pointed out in a comment for D49754, register spilling will currently
spill SPE registers at almost any offset. However, the instructions
evstdd and evldd require a) 8-byte alignment, and b) a limit of 256
(unsigned) bytes from the base register, as the offset must fix into a
5-bit offset, which ranges from 0-31 (indexed in double-words).

This enforces the alignment in offsetMinAlign(), and enforces the spill
range check in PPCRegisterInfo::eliminateFrameIndex().

The update to the register spill test is taken partially from the test
case shown in D49754.

Additionally, pointed out by Kei Thomsen, globals will currently use
evldd/evstdd, though the offset isn't known at compile time, so may
exceed the 8-bit (unsigned) offset permitted. This fixes that as well,
by forcing it to always use evlddx/evstddx when accessing globals.

Part of the patch contributed by Kei Thomsen.

Diff Detail

Event Timeline

jhibbits created this revision.Nov 11 2018, 2:58 PM

I have applied this patch to the llvm-toolchain-7 package in Debian and did not see any regressions on x86_64 or 32-Bit PowerPC. Additionally, I have included the patches from and saw no regressions on x86_64 and 32-bit PowerPC.

All three patches will be part of the next upload of the llvm-toolchain-7 package in Debian unstable which will be version 1:7.0.1~+rc2-9.

nemanjai accepted this revision.Dec 29 2018, 1:43 PM

Other than the minor nit about the test case, LGTM.


Can you maybe filter this test case through something like opt -mem2reg to get rid of the extraneous alloca's to aid readability?

This revision is now accepted and ready to land.Dec 29 2018, 1:43 PM

Other than the minor nit about the test case, LGTM.

Sorry for hi-jacking this, but could you maybe also review this small PowerPC-related change?

jhibbits updated this revision to Diff 185484.Feb 5 2019, 8:31 PM
jhibbits retitled this revision from PowerPC/SPE: Fix register spilling for SPE registers to PowerPC/SPE: Fix load/store handling for SPE.
jhibbits edited the summary of this revision. (Show Details)

Incorporate patch from @kthomsen for handling globals.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 8:31 PM
jhibbits requested review of this revision.Feb 19 2019, 12:40 PM

I thought this would've been auto-marked as needs review when I updated, but apparently not.

nemanjai requested changes to this revision.Feb 20 2019, 3:46 AM

I can't really review this without context. Could you please re-upload with context?

This revision now requires changes to proceed.Feb 20 2019, 3:46 AM
jhibbits updated this revision to Diff 188755.Feb 28 2019, 10:31 AM

Address feedback. Provide full context for diffs.

nemanjai requested changes to this revision.Mar 31 2019, 5:14 AM

I am only requesting changes for the refactoring of the function. As I mentioned in the inline comment, I am not necessarily requesting that you rework the addressing mode computation functions as I mentioned, just that you clean up the code in SelectAddressRegReg().


I really don't think this belongs here. This should probably be in another function. Perhaps something like SelectAddressEVXRegReg(...). This function would need to decide when r+r mode needs to be used.

Furthermore, it is probably buggy that we use iaddr for EVLDD because it might go into a function like this, the node that computes the address is not an ISD::ADD and it reverts to default handling (which is set up to handle 16-bit displacements). Even if you added the code to the ISD::OR section below, it is possible that we would get it wrong as the address came from a different node that SelectAddressRegImm() ends up knowing about.

Ultimately, this is a different addressing mode and it should use different functions to compute when to use the r+r form and when to use r+i form.

That being said, I understand that this solves a current problem whereas what I am suggesting solves a future problem that may never actually occur. So I won't stand in the way of this patch - you just might want to think about re-working this.

At the very least, this code needs to be pulled out into a separate function and this can look like:

if (hasSPE() && usedForDPMemOp(N)) {
  Base = N.getOperand(0);
  Index = N.getOperand(1);
  return true;

This is not necessary. You should be able to do something like:

if (MemSDNode *Memop = dyn_cast<MemSDNode>(*UI)) {
  if (Memop->getMemoryVT() != MVT::f64)
    // ...

A more descriptive name might be something like OffsetFitsMnemonic. Notice the capitalization of the variable name as per coding guidelines.

This revision now requires changes to proceed.Mar 31 2019, 5:14 AM
jhibbits marked 2 inline comments as done.Apr 29 2019, 7:26 AM
jhibbits added inline comments.

Oops, I always forget about the capitalizations. Sorry.