Page MenuHomePhabricator

Extract a TBAAVerifier out of the verifier (NFC)
ClosedPublic

Authored by mehdi_amini on Dec 15 2016, 6:28 PM.

Details

Summary

This is intended to be used (in a later patch) by the BitcodeReader
to detect invalid TBAA and drop them when loading bitcode, so that
we don't break client that have legacy bitcode with possible invalid
TBAA.

Diff Detail

Repository
rL LLVM

Event Timeline

mehdi_amini retitled this revision from to Extract a TBAAVerifier out of the verifier (NFC).
mehdi_amini updated this object.
mehdi_amini added a reviewer: sanjoy.
sanjoy accepted this revision.Dec 15 2016, 6:31 PM
sanjoy edited edge metadata.

This is just code motion, right? If so LGTM.

This revision is now accepted and ready to land.Dec 15 2016, 6:31 PM
mehdi_amini edited edge metadata.

The patch was partial (I didn't squash properly before), here is the full one (hopefully)

This is just code motion, right? If so LGTM.

Mostly: it is moving the code outside of the anonymous namespace and grouping it in its own class, but yes the body of the function is unchanged, except that visitTBAAMetadata also returns a boolean now (ignored by the verifier).

I didn't upload the full patch before (didn't squash the early refactoring), you may want to look again.

lgtm but with one nit inline

llvm/lib/IR/Verifier.cpp
4472 ↗(On Diff #81708)

I assume you'll add a this->HasFailed = true type thing here later?

Also, I'd mildly prefer using this->Diagnostic instead of Diagnostic, since this is in a macro.

Actually, why is this a macro at all? Can't it be a normal templated function just like VerifierSupport::CheckFailed?

mehdi_amini added inline comments.Dec 15 2016, 9:07 PM
llvm/lib/IR/Verifier.cpp
4472 ↗(On Diff #81708)

I assume you'll add a this->HasFailed = true type thing here later?

Not in my plan? I expect to always return false (from the AssertTBAA macro on invalid). I sent you D27839 so you can see how it is used in the Reader.

Actually, why is this a macro at all? Can't it be a normal templated function just like VerifierSupport::CheckFailed?

Good point! Done.

This revision was automatically updated to reflect the committed changes.
sanjoy added inline comments.Dec 16 2016, 11:16 AM
llvm/trunk/include/llvm/IR/Verifier.h
68

Minor typo: "return false if an"