This is an archive of the discontinued LLVM Phabricator instance.

[CodeGenPrepare] Don't sink non-cheap addrspacecasts.
ClosedPublic

Authored by jlebar on Nov 21 2016, 10:22 AM.

Details

Summary

Previously, CGP would unconditionally sink addrspacecast instructions,
even going so far as to sink them into a loop.

Now we check that the cast is "cheap", as defined by TLI.

We introduce a new "is-cheap" function to TLI rather than using
isNopAddrSpaceCast because some GPU platforms want the ability to ask
for non-nop casts to be sunk.

Event Timeline

jlebar updated this revision to Diff 78741.Nov 21 2016, 10:22 AM
jlebar retitled this revision from to [CodeGenPrepare] Don't sink non-cheap addrspacecasts..
jlebar updated this object.
jlebar added reviewers: arsenm, tra.
jlebar added a subscriber: llvm-commits.
tra accepted this revision.Nov 21 2016, 10:50 AM
tra edited edge metadata.
tra added inline comments.
llvm/test/Transforms/CodeGenPrepare/NVPTX/dont-sink-nop-addrspacecast.ll
15

Could that be changed for a positive check at the location where you *do* want to see that addrspacecast?

This revision is now accepted and ready to land.Nov 21 2016, 10:50 AM
jlebar added inline comments.Nov 21 2016, 10:51 AM
llvm/test/Transforms/CodeGenPrepare/NVPTX/dont-sink-nop-addrspacecast.ll
15

What problem would this solve?

tra added inline comments.Nov 21 2016, 11:10 AM
llvm/test/Transforms/CodeGenPrepare/NVPTX/dont-sink-nop-addrspacecast.ll
15

Not having addrspacecast here does not prove that it's where it should be.
I don't know what exactly codegenprepare does, but what if sinking of addrspacecast did happen and for whatever reason it created its own BB just ahead of l2? The test would pass. This scenario is may be too far-fetched, so I'm OK with the test either way.

jlebar updated this revision to Diff 78745.Nov 21 2016, 11:36 AM
jlebar edited edge metadata.

Update test.

llvm/test/Transforms/CodeGenPrepare/NVPTX/dont-sink-nop-addrspacecast.ll
15

No, I think that's reasonable. I added explicit checks that I think should catch this case, thanks.

@arsenm, I would still like your review on this, since you're the impetus for isCheapAddrSpaceCast as opposed to isNoopAddrSpaceCast.

jlebar edited edge metadata.Nov 21 2016, 11:40 AM
This revision now requires review to proceed.Nov 21 2016, 11:40 AM
arsenm accepted this revision.Nov 21 2016, 11:49 AM
arsenm edited edge metadata.

LGTM

llvm/lib/CodeGen/CodeGenPrepare.cpp
829–832

Braces

This revision is now accepted and ready to land.Nov 21 2016, 11:49 AM
jlebar added inline comments.Nov 21 2016, 11:51 AM
llvm/lib/CodeGen/CodeGenPrepare.cpp
829–832

I thought we always omitted them if legal?

(I am very happy to adopt a different rule; I don't like this one at all, fwiw.)

This revision was automatically updated to reflect the committed changes.
jlebar marked 2 inline comments as done.