Page MenuHomePhabricator

GVH-hoist: only clone GEPs (PR28606)
ClosedPublic

Authored by sebpop on Jul 21 2016, 2:51 PM.

Details

Summary

We should not clone stored values unless they are GEPs that are special cased to avoid hoisting them without hoisting their associated ld/st.

Diff Detail

Repository
rL LLVM

Event Timeline

sebpop updated this revision to Diff 64967.Jul 21 2016, 2:51 PM
sebpop retitled this revision from to GVH-hoist: only clone GEPs (PR28606) .
sebpop updated this object.
sebpop added reviewers: majnemer, hans.
sebpop added a subscriber: llvm-commits.

This patch passes make check. I am currently testing this on the test-suite.

majnemer added inline comments.Jul 21 2016, 2:56 PM
llvm/lib/Transforms/Scalar/GVNHoist.cpp
586 ↗(On Diff #64967)

We could put the check here:

...
GetElementPtrInst *Val = nullptr;
if (auto *St = dyn_cast<StoreInst>(Repl)) {
  Gep = ...
  Val = dyn_cast<GetElementPtrInst>(St->getValueOperand());
sebpop added inline comments.Jul 21 2016, 3:04 PM
llvm/lib/Transforms/Scalar/GVNHoist.cpp
586 ↗(On Diff #64967)

I think this is not possible: we need to check whether the stored value is available at HoistPt.
If we dyn_cast Val to GEP this early we would only hoist GEP stores.

majnemer added inline comments.Jul 21 2016, 3:07 PM
llvm/lib/Transforms/Scalar/GVNHoist.cpp
586 ↗(On Diff #64967)

Oh, I understand now. You are trying to move the address computation closer to the memory operation...

592–594 ↗(On Diff #64967)

This can be removed now, we won't try to clone it.

sebpop updated this revision to Diff 64974.Jul 21 2016, 3:19 PM

Also remove the check for stored PHI nodes.

majnemer accepted this revision.Jul 21 2016, 3:24 PM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jul 21 2016, 3:24 PM
eli.friedman added inline comments.
llvm/lib/Transforms/Scalar/GVNHoist.cpp
588 ↗(On Diff #64974)

I think you want "Val && !DT->dominates(Val->getParent(), HoistPt)", or something like that? You care whether the value itself is available, not its operands.

sebpop added inline comments.Jul 21 2016, 3:53 PM
llvm/lib/Transforms/Scalar/GVNHoist.cpp
588 ↗(On Diff #64974)

This is correct. We do not need to check all operands of the Val instruction.

sebpop updated this revision to Diff 64986.Jul 21 2016, 4:17 PM
sebpop edited edge metadata.

For everything except GEPs, only check that the stored value is available: do not check the availability of the operands.

You're missing an allOperandsAvailable call if Val is a GEP. (I'm not sure why you're even trying to special-case GEPs here.)

This revision was automatically updated to reflect the committed changes.

You're missing an allOperandsAvailable call if Val is a GEP. (I'm not sure why you're even trying to special-case GEPs here.)

GEPs are hoisted only when the ld or st can be moved.

I added that check.