This is an archive of the discontinued LLVM Phabricator instance.

[OpaquePtrs] Add getNonOpaquePointerElementType() method
ClosedPublic

Authored by nikic on Jan 21 2022, 3:55 AM.

Details

Summary

This method is intended for use in places that cannot be reached with opaque pointers, or part of deprecated methods. This makes it easier to see that some uses of getPointerElementType() don't need further action.

I would also like to deprecate PointerType::getElementType() in favor of Type::getPointerElementType() in a followup. The PointerType::getElementType() method is annoying in that you cannot see at a glance whether this is actually accessing a pointer element type, a vector element type or an array element type. I'd like to reduce our TODO list to grep getPointerElementType.

Diff Detail

Event Timeline

nikic created this revision.Jan 21 2022, 3:55 AM
nikic requested review of this revision.Jan 21 2022, 3:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 3:55 AM
aeubanks accepted this revision.Jan 21 2022, 10:34 AM
This revision is now accepted and ready to land.Jan 21 2022, 10:34 AM

LGTM too; just wanted to share an optional suggestion.

llvm/include/llvm/IR/Type.h
375–378

Given where this should be used, should all uses be required to be documented with a FIXME/TODO/SOMETHING to indicate which of the two cases it is?

I don't feel strongly (up to you), but it might make it easier to audit them.

llvm/lib/AsmParser/LLParser.cpp
1413–1414

E.g., this could have:

// NOTE: getNonOpaquePointerElementType() call unreachable with opaque
// pointers.

I /very/ vaguely remember having picked one of "getPointerElementType" and "getElementType" as a sort of intentional transformation like this (though adding a new explicit name's a further step along that direction) back when I started the opaque pointer work to help me classify things I'd cleaned up and things I hadn't... (I think I put an assert(false) in the one I hadn't cleaned up to help me find places I still needed to clean up) but I think it's too far back in my brain.

Happy with whatever you/other folks feels good here.

llvm/include/llvm/IR/Type.h
375–378

Yeah, or possibly have two different function names to self-document those two cases.

nikic added inline comments.Jan 22 2022, 12:24 AM
llvm/include/llvm/IR/Type.h
375–378

My 2c here is that it doesn't really matter why the method is used, just that the code will get deleted after the migration.

I think it's fairly obvious in most cases, because it's either directly in a deprecated API, or shortly after an isOpaque() check. A comment might be warranted if the check is far removed (e.g. the SROA case is a bit non-obvious, because the opaque pointer check happens in a different function.)

dblaikie added inline comments.Jan 22 2022, 10:59 AM
llvm/include/llvm/IR/Type.h
375–378

Ah, it sounded like there are two classes here that will need to be treated differently - like the deprecated uses we would expect to cleanup before the opaque pointer flag flip, but the "not reachable" cases would not be cleaned up before that flip? (or the other way around, or some other variation) Is that not the case? Could you expand a bit on the difference between the two cases described by this comment, and the expected lifecycle/path for them?

nikic added inline comments.Jan 22 2022, 11:29 AM
llvm/include/llvm/IR/Type.h
375–378

In either case, the calls would have to go away when typed pointer support is dropped. For uses in deprecated methods they *might* go away earlier -- this depends on whether we're going to drop C API support in advance or not. I don't think we have a decision on that yet.

dblaikie accepted this revision.Jan 23 2022, 10:50 AM
dblaikie added inline comments.
llvm/include/llvm/IR/Type.h
375–378

Fair enough - thanks for the explanation!

This revision was landed with ongoing or failed builds.Jan 24 2022, 1:03 AM
This revision was automatically updated to reflect the committed changes.