This is an archive of the discontinued LLVM Phabricator instance.

[MemorySSA] Extend the clobber walker with the option to skip the starting access.
ClosedPublic

Authored by asbirlea on Dec 20 2018, 12:01 PM.

Details

Summary

The option enables loop transformations to hoist accesses that do not
have clobbers in the loop. If the clobber queries skips the starting
access, the result may be outside the loop instead of the header Phi.

Adding the walker that uses this option in a separate patch.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Dec 20 2018, 12:01 PM

Thanks for this! I like it when patches are small and split up like this one :)

I'd rather not check in dynamically dead code, though. Can we please hold off on committing this until the patch that makes this un-dead is approved and such?

lib/Analysis/MemorySSA.cpp
625 ↗(On Diff #179116)

Is it sensible to have SkipSelfAccess with Query->OriginalAccess == null?

628 ↗(On Diff #179116)

Can we move this assert where we plan to set Query->SkipSelfAccess to true?

That way, it'll fire 100% of the time if there's a bug, rather than only if we reach this piece of code

(I realize that'll be in the follow-up patch. Still. :) )

650 ↗(On Diff #179116)

Please add a short comment explaining why this if exists, since I feel like I'm likely to forget the trickiness we're doing here, and it would help any future readers of the code.

asbirlea updated this revision to Diff 179897.Jan 2 2019, 11:10 AM
asbirlea marked 5 inline comments as done.

Address comments.

Ack, will hold off on checking this in until the follow-up use (walker) is finalized.

lib/Analysis/MemorySSA.cpp
625 ↗(On Diff #179116)

AFAICT Query->OriginalAccess is never null, so no, not currently. Removing check.

628 ↗(On Diff #179116)

Ack, noted to include in follow-up patch.

asbirlea updated this revision to Diff 180169.Jan 3 2019, 3:45 PM

Update setting of where to stop in getBlockingAccess:

  • dropping the check for block since we may no longer be in the starting block after adding a paused path.
  • adding a second StopAt access in walkToPhiOrClobber, only set when skipping self and for the same location.
  • keeping paused path when the original StopAt is found.
george.burgess.iv accepted this revision.Jan 4 2019, 9:39 AM

LGTM; thanks again.

When we have code in a transformation that uses this new walker, please include a test specifically for the SkipStopWhere case that your most recent patchset fixes.

This revision is now accepted and ready to land.Jan 4 2019, 9:39 AM

Thank you for the review!

Yes, I already have a testcase to include in the patch using the new walker.

This revision was automatically updated to reflect the committed changes.