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

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–589

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–589

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–589

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

591–592

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

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

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.