This is an archive of the discontinued LLVM Phabricator instance.

Tie the Verifier class to a Module; NFCI
ClosedPublic

Authored by sanjoy on Aug 1 2016, 4:52 PM.

Details

Summary

This commit changes the Verifier class to accept a Module via the
constructor to make it obvious that a specific instance of the class is
only intended to work with a specific module. The updateModule setter
(despite being private) was making this fact less transparent.

There are fields in the Verifier class like DeoptimizeDeclarations
and GlobalValueVisited which are module specific, so a given
Verifier instance will not in fact work across multiple modules today.
This change just makes that more obvious.

The motivation is to make it easy to get to the datalayout of the
module unambiguously. That is required to verify that inttoptr and
ptrtoint constant expressions are well typed in the face of
non-integral pointer types.

I'm also tempted to change the Verifier::verify(const Module &M)
function to Verifier::verify() (as a separate change perhaps?) if
people are on-board with that.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 66402.Aug 1 2016, 4:52 PM
sanjoy retitled this revision from to Tie the Verifier class to a Module; NFCI.
sanjoy updated this object.
sanjoy added reviewers: dexonsmith, bkramer, chandlerc.
sanjoy added a subscriber: llvm-commits.
majnemer added inline comments.
lib/IR/Verifier.cpp
88 ↗(On Diff #66402)

Should this be a reference now?

256–257 ↗(On Diff #66402)

Should we remove this M parameter now?

sanjoy updated this object.Aug 1 2016, 5:07 PM
sanjoy added a reviewer: majnemer.
sanjoy removed a subscriber: majnemer.

Agree with both of your points, but do you mind if I do those in the next change? That way this change stays small and obvious.

mehdi_amini added inline comments.
lib/IR/Verifier.cpp
310 ↗(On Diff #66402)

I'd just remove the Module parameter as part of the same patch. It becomes a sketchy invariant to be part of the API now.

chandlerc accepted this revision.Aug 1 2016, 5:21 PM
chandlerc edited edge metadata.

FWIW, LGTM regardless of how you sequence the changes, but I like the changes suggested by David and Mehdi. Feel free to sort out with them here or IRC how to land this.

lib/IR/Verifier.cpp
310 ↗(On Diff #66402)

+1

This revision is now accepted and ready to land.Aug 1 2016, 5:21 PM
sanjoy updated this revision to Diff 66410.Aug 1 2016, 5:47 PM
sanjoy edited edge metadata.

Address review:

  • Remove "false" parameters from Verifier::verify(Module) and Verify::checkAtomicMemAccessSize.
  • Minor simplifications resulting from the change that are not worth the hassle to separately land.

I will change the Module and LLVMContext in VerifierSupport to be
references in a later change.

mehdi_amini added inline comments.Aug 1 2016, 6:03 PM
lib/IR/Verifier.cpp
91 ↗(On Diff #66410)

Is the Context member used?

sanjoy added inline comments.Aug 1 2016, 6:38 PM
lib/IR/Verifier.cpp
91 ↗(On Diff #66410)

Yes, it is used in Verifier (which inherits from VerifierSupport).

This revision was automatically updated to reflect the committed changes.