This is an archive of the discontinued LLVM Phabricator instance.

[OpaquePtr] Handle addrspacecasts in InstCombine
ClosedPublic

Authored by nikic on Jun 21 2021, 1:39 PM.

Details

Summary

This adds support for addrspace casts involving opaque pointers to InstCombine, as well as the isEliminableCastPair() helper (otherwise the assertion failure would just move there).

Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.

Diff Detail

Event Timeline

nikic created this revision.Jun 21 2021, 1:39 PM
nikic requested review of this revision.Jun 21 2021, 1:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2021, 1:39 PM
nikic updated this revision to Diff 353494.Jun 21 2021, 2:31 PM

Use getWithSamePointeeType().

nikic added a comment.Jun 21 2021, 2:40 PM

Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.

Or possibly PointerType::hasSameElementTypeAs(PointerType *) would make sense here.

Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.

Or possibly PointerType::hasSameElementTypeAs(PointerType *) would make sense here.

I think PointerType::hasSameElementTypeAs(PointerType *) is good, we already have weird PointerType methods that are only for the opaque pointer transition, like PointerType::getWithSamePointeeType(), that aren't used in a lot of places and are to be deleted after.

Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.

Or possibly PointerType::hasSameElementTypeAs(PointerType *) would make sense here.

I think PointerType::hasSameElementTypeAs(PointerType *) is good, we already have weird PointerType methods that are only for the opaque pointer transition, like PointerType::getWithSamePointeeType(), that aren't used in a lot of places and are to be deleted after.

*thumbs up* I hope these sort of helpers will make the transition a bit easier - they can become no-ops/have their non-opaque parts removed at the appropriate point in the transition, then simple/clear/obvious change to remove the function and have callers do the right thing in a cleanup/NFC change.

nikic updated this revision to Diff 353573.Jun 22 2021, 1:02 AM

Add PointerType::hasSameElementTypeAs().

aeubanks accepted this revision.Jun 22 2021, 8:26 AM

lgtm with one nit

llvm/include/llvm/IR/DerivedTypes.h
701 ↗(On Diff #353573)

could you add a note that this is purely for the opaque pointer transition

This revision is now accepted and ready to land.Jun 22 2021, 8:26 AM
This revision was automatically updated to reflect the committed changes.