This is an archive of the discontinued LLVM Phabricator instance.

[OpaquePtr] Make cmpxchg work with opaque pointers
ClosedPublic

Authored by aeubanks on May 18 2021, 9:05 PM.

Diff Detail

Event Timeline

aeubanks created this revision.May 18 2021, 9:05 PM
aeubanks requested review of this revision.May 18 2021, 9:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2021, 9:05 PM
dblaikie added inline comments.May 19 2021, 11:18 AM
llvm/lib/IR/Verifier.cpp
3854–3856

Is there missing testing for this? Looks like the wording/assert was changed, but no test cases were updated for this?

aeubanks added inline comments.May 19 2021, 11:24 AM
llvm/lib/IR/Verifier.cpp
3854–3856

I think we could ever reasonably reach these since the bitcode reader and llparser will both already give up if this were to fail. And there's a similar assert when actually creating the instructions.
So I'm not sure how to actually test this. Maybe we should just remove these?

dblaikie added inline comments.May 19 2021, 11:32 AM
llvm/lib/IR/Verifier.cpp
3854–3856

Yeah, if they're unreachable I'd be inclined to remove them.

The only thing that comes to mind would be in a non-asserts build, using a unit test/API testing - build it then verify. But that's testing behavior that's out-of-contract (behavior behind a failed assertion) anyway, which isn't reasonable in my opinion.

So, yeah, if you can't get there in a +Asserts build via API, bitcode, or textual IR, I'd say it's dead code & be fine with dropping it (probably do that in a separate preliminary commit, though)

dblaikie accepted this revision.May 19 2021, 11:32 AM

Approval for this patch/the rest of it, however the dead Asserts are handled.

This revision is now accepted and ready to land.May 19 2021, 11:32 AM
aeubanks added inline comments.May 19 2021, 12:41 PM
llvm/lib/IR/Verifier.cpp
3854–3856

sounds good, I'll delete these in a future change

This revision was automatically updated to reflect the committed changes.