This is an archive of the discontinued LLVM Phabricator instance.

[CallSite removal] Move the verifier to use `CallBase` instead of the `CallSite` wrapper.
ClosedPublic

Authored by chandlerc on Dec 28 2018, 8:09 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Dec 28 2018, 8:09 PM
compnerd accepted this revision.Dec 31 2018, 10:38 PM
compnerd added a subscriber: compnerd.
compnerd added inline comments.
llvm/lib/IR/Verifier.cpp
492 ↗(On Diff #179673)

I think it would be nicer to call it verifyCall.

2858 ↗(On Diff #179673)

I think that this might be easier to read:

if (!Call.getCalledFunction() || Call.getCalledFunction()->getIntrinsicID() != Intrinsic:::experimental_gc_statepoint)
2872 ↗(On Diff #179673)

Similar, use the negation than the nullptr check

2883 ↗(On Diff #179673)

Similar

4129 ↗(On Diff #179673)

Can't this be const?

4394 ↗(On Diff #179673)

Can't this be const?

4418 ↗(On Diff #179673)

Mind switching to static_cast<int>? -Wold-style-cast is .... annoying.

This revision is now accepted and ready to land.Dec 31 2018, 10:38 PM
chandlerc updated this revision to Diff 179827.Jan 2 2019, 2:47 AM
chandlerc marked 8 inline comments as done.

Address review feedback.

Thanks for the review, see comments inline.

llvm/lib/IR/Verifier.cpp
492 ↗(On Diff #179673)

I made it CallBase to be consistent with CallInst and InvokeInst, but then I didn't actually make it override the inst visitor method and put it with that list. I've fixed that now. Does this make more sense out of its name?

2858 ↗(On Diff #179673)

Done, but again, these were mechanical changes. I don't know that I can fix all the bad code that happens to be touched.

4129 ↗(On Diff #179673)

It wasn't before my change, so I'd rather not introduce the potential for still more deltas to code.

4394 ↗(On Diff #179673)

See above.

4418 ↗(On Diff #179673)

Not really.

We have *so many* of these in the code base that I don't think this is worth doing generally. And moreover, I don't think this is the patch to try and address that.

Confirmed w/ compnerd that this is fine and landing.

This revision was automatically updated to reflect the committed changes.