This is an archive of the discontinued LLVM Phabricator instance.

[SLP] - Add couple safety checks to TreeEntry::dump(). NFC
ClosedPublic

Authored by vdmitrie on Nov 4 2019, 10:28 AM.

Details

Summary

Check for MainOp and AltOp for NULL before dereferencing or issue NULL.

Diff Detail

Event Timeline

vdmitrie created this revision.Nov 4 2019, 10:28 AM

Tests?

Do you have any ideas how a test might look?

Tests?

Do you have any ideas how a test might look?

unittests?

Do you have any ideas how a test might look?

unittests?

I did not mean test's shape when asked the question. This dump routine only called from dumpVectorizableTree routine which in turn is never called in the code. It exists to support debugging.
I can imagine adding "gdb --args opt ..." test with gdb script to call mentioned routine but think it is overkill for this sort of change.
It would be great if you could point an existing test case that intended to test similar issue.

Something like llvm/unittests/IR/DominatorTreeBatchUpdatesTest.cpp?

Something like llvm/unittests/IR/DominatorTreeBatchUpdatesTest.cpp?

Thank you Sir, but the test you pointed looks overcomplicated for the purpose of testing this debug printer. Why did not you request unit test for example here (https://reviews.llvm.org/D59059) when the dump routine was initially implemented?

As a LIT test it could look like this:

But it cannot be added this way.

Update: I have spent some time investigating unit tests infra and finally came to conclusion that it is not quite possible to create a unit test that calls BoUpSLP::dumpVectorizableTree() as we need to construct BoUpSLP object. We are unable to do that having class forward declaration only.

This revision is now accepted and ready to land.Nov 4 2019, 7:05 PM
This revision was automatically updated to reflect the committed changes.