This is an archive of the discontinued LLVM Phabricator instance.

Add functions to MachineLICM to hoist invariant stores.
ClosedPublic

Authored by syzaara on Nov 17 2017, 1:45 PM.

Details

Summary

This patch adds functions to allow MachineLICM to hoist invariant stores. Currently, MachineLICM does not hoist any store instructions, however when storing the same value to a constant spot on the stack, the store instruction should be considered invariant and be hoisted.
Such stores are common with indirect calls on PowerPC where we save the TOC pointer in R2 to a constant spot on the stack. R2 is constant throughout the function.

The function isInvariantStore iterates each operand of the store instruction and checks that each register operand satisfies isCallerPreservedPhysReg.

These store instructions are fed by a copy:
%vreg12<def> = COPY %X2; G8RC:%vreg12
STD %vreg12, 24, %X1; mem:ST8[Stack+24] G8RC:%vreg12

When hoisting the store, the copy should be hoisted as well. This is handled with isCopyFeedingInvariantStore.

For more details, please see RFC: http://lists.llvm.org/pipermail/llvm-dev/2017-November/119116.html

Diff Detail

Repository
rL LLVM

Event Timeline

syzaara created this revision.Nov 17 2017, 1:45 PM
syzaara retitled this revision from [PowerPC] Add hooks to MachineLICM to hoist invariant stores. to Add hooks to MachineLICM to hoist invariant stores..Nov 17 2017, 3:57 PM
syzaara edited the summary of this revision. (Show Details)
nemanjai set the repository for this revision to rL LLVM.

Hoisting these invariant stores is certainly something we want on PPC. It makes sense that MachineLICM is the place to do it. I've added a few people that I think are familiar with the pass to help with the review.

lib/CodeGen/MachineLICM.cpp
717 ↗(On Diff #123409)

Looks like this is just

// If we have hoisted an instruction that may store, it can only be a
// constant store.
else if (MI->mayStore())
  NumStoreConst++;
lib/CodeGen/TargetInstrInfo.cpp
890 ↗(On Diff #123409)

It should be safe to not have this condition (and perhaps just turn it into an assert).

891 ↗(On Diff #123409)

Why not for (MachineInstr &UseMI : use_instructions(CopyDstReg))?

qcolombet added inline comments.
include/llvm/CodeGen/TargetInstrInfo.h
1133 ↗(On Diff #123409)

Instead of patching every single optimization with this new hook, could you teach isSafeToMove this concept of invariant store?

1136 ↗(On Diff #123409)

Why does this need to be a TII hook?

MatzeB edited edge metadata.Nov 29 2017, 12:06 PM

Didn't we already add TargetRegisterInfo::isCallerPreservedPhysReg() specifically for this PPC issue?

include/llvm/CodeGen/TargetInstrInfo.h
1133 ↗(On Diff #123409)

This should also have more detailed explanations! Can you really move the store anywhere in the function? (As an example: Would in front of the prologue or before a return be still okay? I suspect not.)

MatzeB added a reviewer: lei.Nov 29 2017, 12:07 PM
syzaara added inline comments.Nov 29 2017, 12:46 PM
include/llvm/CodeGen/TargetInstrInfo.h
1133 ↗(On Diff #123409)

There are other optimizations that use isSafeToMove where it does not make sense to teach it about invariant stores. For example, DeadMachineInstructionElim.cpp will end up deleting the store instruction if isSafeToMove was modified. We want to restrict what we do with these invariant stores to MachineLICM.

Didn't we already add TargetRegisterInfo::isCallerPreservedPhysReg() specifically for this PPC issue?

isCallerPreservedPhysReg was added to allow hoisting of load instructions whose operands were preserved physical regs.
In this case, we are trying to hoist a store instruction, which currently is not supported by MachineLICM. MachineLICM considers all stores unsafe to hoist.
This teaches MachineLICM to allow hoisting of invariant stores when all the operands of the store satisfy isCallerPreservedPhysReg.

lei added inline comments.Nov 30 2017, 12:48 PM
lib/CodeGen/MachineLICM.cpp
892 ↗(On Diff #123409)

nit: since HoistConstStores is a boolean controlled by option, I'd put this as the first ck in this && cond.

1112 ↗(On Diff #123409)

I don't think you need to ck for MI.isCopy() here since isCopyFeedingInvariantStore() checks for this and does an early exit if it's false.

lib/Target/PowerPC/PPCInstrInfo.cpp
302 ↗(On Diff #123409)

Please add documentation for the following 2 functions.

330 ↗(On Diff #123409)

why not use: for (const MachineOperand &MO : MI.operands())

lib/Target/PowerPC/PPCRegisterInfo.cpp
304 ↗(On Diff #123409)

Need more explanation as to why X1 is caller preserved.

syzaara updated this revision to Diff 125601.Dec 5 2017, 1:36 PM
hfinkel added inline comments.Dec 13 2017, 12:27 PM
include/llvm/CodeGen/TargetInstrInfo.h
1135 ↗(On Diff #125601)

Document here whether or not it is legal to call this on instructions that aren't stores.

lib/CodeGen/MachineLICM.cpp
890 ↗(On Diff #125601)

If we decide to move the mayStore check into isInvariantStore, then remove it here.

1098 ↗(On Diff #125601)

This really seems to represent the way that the PPC implementation of isInvariantStore works. What if other targets look through things that aren't pure copies? Your PPC implementation of isInvariantStore could be target independent. I'd just move it here to be next to this implementation. (This won't affect any other targets anyway, because only PPC currently implements isCallerPreservedPhysReg).

lib/Target/PowerPC/PPCInstrInfo.cpp
308 ↗(On Diff #125601)

This is going to be added to PPCInstInfo in D31852, don't add it twice :-)

335 ↗(On Diff #125601)

Add something here to validate that we've been called on an instruction that might store?

assert(MI->mayStore() && "isInvariantStore not called on a store instruction");

or put the check here:

if (!MI->mayStore())
  return false;

depending on the contract we want.

lib/Target/PowerPC/PPCRegisterInfo.cpp
69 ↗(On Diff #125601)

I don't see any test cases for this. Can you add one?

296 ↗(On Diff #125601)

This applies to ELFv1 too, please don't restrict this to ELFv2.

syzaara updated this revision to Diff 128028.Dec 22 2017, 8:03 AM
syzaara added inline comments.Dec 22 2017, 8:05 AM
lib/Target/PowerPC/PPCRegisterInfo.cpp
69 ↗(On Diff #125601)

The testcase added test/CodeGen/PowerPC/loop-hoist-toc-save.ll tests for -ppc-stack-ptr-caller-preserved in combination with hoist-const-stores.

hfinkel accepted this revision.Jan 7 2018, 12:04 PM

LGTM

This revision is now accepted and ready to land.Jan 7 2018, 12:04 PM

@echristo Hi Eric, do you have any comments regarding this patch?

A few comments inline.

include/llvm/CodeGen/TargetInstrInfo.h
303 ↗(On Diff #128028)

This API is described a little awkwardly and you can see it from the comment - "... for a Machine instruction" and the API doesn't take an MI, but just a register and MRI.

Maybe instead it should be on TRI?

lib/CodeGen/MachineLICM.cpp
894–897 ↗(On Diff #128028)

I understand the desire to make these functions static, but you should probably either pass in the information you need into them or make them class functions. There's no need to look up all of this information in a loop for sure when we have it cached in the class.

926–929 ↗(On Diff #128028)

Should possibly just be a class function or take the variables it needs as arguments rather than grabbing them.

lib/CodeGen/TargetInstrInfo.cpp
301 ↗(On Diff #128028)

const?

syzaara updated this revision to Diff 133243.Feb 7 2018, 10:04 AM

@echristo Hi Eric, are you okay to approve this now?

@echristo Hi Eric, are you okay to approve this now?

You didn't answer any of my questions?

syzaara updated this revision to Diff 134061.Feb 13 2018, 9:32 AM

@echristo Hi Eric, are you okay to approve this now?

You didn't answer any of my questions?

Sorry I missed the first point. I agree, I think it makes more sense to move lookThruCopyLike to TRI rather than TII.

echristo accepted this revision.Mar 13 2018, 12:53 PM

Thanks!

-eric

syzaara retitled this revision from Add hooks to MachineLICM to hoist invariant stores. to Add functions to MachineLICM to hoist invariant stores..Mar 15 2018, 9:01 AM
syzaara edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.