This is an archive of the discontinued LLVM Phabricator instance.

[OpaquePtr] Make loads and stores work with opaque pointers
ClosedPublic

Authored by aeubanks on May 13 2021, 3:51 PM.

Details

Summary

Don't check that types match when the pointer operand is an opaque
pointer.

I would separate the Assembler and Verifier changes, but
verify-uselistorder in the Assembler test ends up running the verifier.

Diff Detail

Event Timeline

aeubanks created this revision.May 13 2021, 3:51 PM
aeubanks requested review of this revision.May 13 2021, 3:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2021, 3:51 PM
dblaikie added inline comments.May 13 2021, 5:44 PM
llvm/lib/AsmParser/LLParser.cpp
7533–7534

Reckon there's anything we could do to generalize these sort of tests - if we're going to add a whole bunch of them through this migration (& then remove them once we've moved over to opaque pointer types), maybe there are some good primitives we could create?

Like "hasElementTypeOrOpaque(PtrType, PointeeType)"?

aeubanks updated this revision to Diff 345334.May 13 2021, 6:54 PM

factor out check for pointee type
add some more missed places (not sure why I thought the test was passing before?)

dblaikie added inline comments.May 13 2021, 7:04 PM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3849 ↗(On Diff #345334)

Is this for sure equivalent to the old code? The old code looks like it's equivalent to PointerType::isValidElementType which looks different to me to isFirstClassType?

llvm/lib/IR/Verifier.cpp
3749

What's this condition for? If this is true, isOpaqueOrPointeeTypeMatches would be true and the assert would not fail, so that seems OK?

aeubanks updated this revision to Diff 345341.May 13 2021, 8:17 PM

remove unnecessary check

aeubanks marked an inline comment as done.May 13 2021, 8:23 PM
aeubanks added inline comments.
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3849 ↗(On Diff #345334)

it matches the check in LLParser:

if (!Val->getType()->isPointerTy() || !Ty->isFirstClassType())
  return error(Loc, "load operand must be a pointer to a first class type");

The pointer should already have a valid pointee type, so that check isn't useful. isFirstClassType() seems more proper than just checking for a function type

Matt added a subscriber: Matt.May 14 2021, 8:20 AM
dblaikie added inline comments.May 14 2021, 5:56 PM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3849 ↗(On Diff #345334)

I'd be inclined to have that as a separate commit, perhaps? (& is it worth an assertion that those other cases covered by isLoadableOrStorableType don't come up here? (demonstrating that these two phrasings are equivalent)

aeubanks updated this revision to Diff 345927.May 17 2021, 10:26 AM

isFunctionType() instead of isFirstClassType()

aeubanks updated this revision to Diff 346214.May 18 2021, 10:53 AM

address comments more

aeubanks added inline comments.May 18 2021, 10:54 AM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3849 ↗(On Diff #345334)

made this closer to how it was originally

dblaikie accepted this revision.May 18 2021, 1:11 PM

Sounds good!

This revision is now accepted and ready to land.May 18 2021, 1:11 PM
This revision was landed with ongoing or failed builds.May 18 2021, 1:48 PM
This revision was automatically updated to reflect the committed changes.