This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Refactor SafepointIRVerifier
ClosedPublic

Authored by DaniilSuchkov on Dec 6 2017, 1:48 AM.

Details

Summary

Now two classes are responsible for verification: one of them can track GC
pointers and know whether a pointer is relocated or not and another based on
that information can verify uses of GC pointers.

Diff Detail

Event Timeline

DaniilSuchkov created this revision.Dec 6 2017, 1:48 AM
mkazantsev added inline comments.Dec 7 2017, 10:17 AM
lib/IR/SafepointIRVerifier.cpp
140

Please, check it in as separate NFC. This is a big patch, and it is hard to follow what is going on while you're mixing refactoring and functional changes.

241

Do you really need this field? It is only used in verifyFunction, why not just pass the function into it as parameter?

245

Please, give it more explanation what do you mean by "can be safely ignored".

365

{} not needed

367

Else is not needed.

372

Could you please add a comment what is going on here?

456–465

How about removeUnrelocatedDefs? It would be more consistent with what debug message in this function is.

DaniilSuchkov added inline comments.Dec 8 2017, 3:10 AM
lib/IR/SafepointIRVerifier.cpp
140

Nothing here is a functional change but I agree that this change should be a separate patch.
https://reviews.llvm.org/D41002

DaniilSuchkov marked 6 inline comments as done.
DaniilSuchkov added inline comments.
lib/IR/SafepointIRVerifier.cpp
241

We need F both in constructor and verifyFunction and it must be the same value so it's better leave it as a field.
Though const DominatorTree &DT isn't required to be field so it'll be removed.

mkazantsev accepted this revision.Dec 9 2017, 1:00 AM

LGTM

lib/IR/SafepointIRVerifier.cpp
490

Should be "transferred", but since it was in the ogirinal code, I'm OK if you fix it in follow-up NFC.

This revision is now accepted and ready to land.Dec 9 2017, 1:00 AM
This revision was automatically updated to reflect the committed changes.