This is an archive of the discontinued LLVM Phabricator instance.

OpaquePtr: Reject 'ptr*' again when parsing textual IR
ClosedPublic

Authored by dexonsmith on Jun 25 2021, 11:56 AM.

Details

Summary

Bring back the testcase dropped in
1e6303e60ca5af4fbe7ca728572fd65666a98271 and get it passing by checking
explicitly for ptr* in LLParser. Added Type::isOpaquePointerTy() for
convenience.

Diff Detail

Event Timeline

dexonsmith created this revision.Jun 25 2021, 11:56 AM
dexonsmith requested review of this revision.Jun 25 2021, 11:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2021, 11:56 AM
nikic accepted this revision.Jun 25 2021, 11:59 AM

LGTM -- looks like we had a bit of race condition, I just committed ad4bb8280952c2cacf497e30560ee94c119b36e0 in preparation for doing the same change...

This revision is now accepted and ready to land.Jun 25 2021, 11:59 AM
aeubanks accepted this revision.Jun 25 2021, 12:02 PM

lgtm but with a comment on the newly added method

llvm/include/llvm/IR/Type.h
231 ↗(On Diff #354567)

I have a mild preference to not add these one off sort of methods when we already have lots of code that does isa<PointerType> && cast<PointerType>(Ty)->isOpaque()

This revision was automatically updated to reflect the committed changes.

Thanks for both reviews! Landed in 4506f614cb6983a16d117cf77a968608e66d7a5c.

LGTM -- looks like we had a bit of race condition, I just committed ad4bb8280952c2cacf497e30560ee94c119b36e0 in preparation for doing the same change...

Yeah, sorry about that / thanks; after I left the other review I realized it was easy enough for me to pitch in here :).

llvm/include/llvm/IR/Type.h
231 ↗(On Diff #354567)

Agreed it shouldn't be a one off, was surprised it hadn't been added yet though... I'll see if I can find time to clean up the existing boilerplate instances to use this helper (note that @nikic already added it).

dexonsmith added inline comments.Jun 25 2021, 3:27 PM
llvm/include/llvm/IR/Type.h
231 ↗(On Diff #354567)

Actually, git-grep can't come up with any examples of the more verbose pattern anymore... looks like @nikic cleaned them all up.

nikic added inline comments.Jun 29 2021, 11:19 AM
llvm/lib/AsmParser/LLParser.cpp
2601

This breaks pointer to pointer types in -force-opaque-pointers mode:

define void @test(i32** %x) {}

Results in:

build/bin/opt: test303.ll:1:23: error: ptr* is invalid - use ptr instead
dexonsmith added inline comments.Jun 29 2021, 12:35 PM
llvm/lib/AsmParser/LLParser.cpp
2601

Looking.

dexonsmith added inline comments.Jun 29 2021, 12:57 PM
llvm/lib/AsmParser/LLParser.cpp
2601