Page MenuHomePhabricator

[OpaquePtr] Create API to make a copy of a PointerType with some address space
ClosedPublic

Authored by aeubanks on Mon, May 31, 3:37 PM.

Details

Summary

Some existing places use getPointerElementType() to create a copy of a
pointer type with some new address space.

Diff Detail

Event Timeline

aeubanks created this revision.Mon, May 31, 3:37 PM
aeubanks requested review of this revision.Mon, May 31, 3:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 31, 3:37 PM
dblaikie added inline comments.Tue, Jun 1, 12:06 PM
llvm/include/llvm/IR/DerivedTypes.h
667

Maybe the "with same pointee type" isn't the high priority aspect of this API? More "get with different address space" and it's assumed the rest of the type remains the same? (opaque to opaque, typed to typed)

Maybe "getWithAddressSpace" or something like that?

aeubanks added inline comments.Tue, Jun 1, 3:32 PM
llvm/include/llvm/IR/DerivedTypes.h
667

There are already a couple PointerType::get() methods which construct a pointer type. This one is different in that it's sorta cloning an existing PointerType. I want to make that super clear via the name somehow. This could be changed to a non-static method to be clearer? IMO Something like getWithAddressSpace() is too similar to the existing methods.

And this should be removed after the opaque pointer transition is over.

dblaikie added inline comments.Tue, Jun 1, 4:02 PM
llvm/include/llvm/IR/DerivedTypes.h
667

Yeah, happy for this to go away after the opaque pointer transition - when nothing significant from the old pointer type would be used to make the new pointer type anyway (since the only interesting property of the opaque pointer is its address space).

I don't too much mind it being a non-static member - it is different in that it's derived from the current pointer type so it can readily be a non-static member function.

dblaikie accepted this revision.Tue, Jun 1, 4:04 PM

Eh, either way - see what you mean about the existing name. And since it'll be relatively short-lived (maybe modify the comment to say "delete this after the opaque pointer transition" rather than the more passive "this is only useful ... " ?) the current name you've got seems OK.

This revision is now accepted and ready to land.Tue, Jun 1, 4:04 PM
This revision was landed with ongoing or failed builds.Tue, Jun 1, 4:52 PM
This revision was automatically updated to reflect the committed changes.