Page MenuHomePhabricator

[ThinLTO] Fix test with reverse-iteration
AbandonedPublic

Authored by Hahnfeld on Feb 19 2019, 7:27 AM.

Details

Summary

Make test more flexible with regard to ordering. Additionally, FileCheck
cannot test -SAME patterns after DAG, so duplicate the line manually.

Diff Detail

Event Timeline

Hahnfeld created this revision.Feb 19 2019, 7:27 AM

Is it expected that the test output is sensitive to reverse iteration?
It sounds to me more like an issue with the code than with how the test is expressed.

Summary clusters, nodes and edges are retrieved from DenseMap with arbitrary order, which doesn't make result DOT file invalid.
Reverse iteration just exposes test issues.

evgeny777 accepted this revision.Feb 19 2019, 9:17 AM

LGTM, but please wait for a while for Teresa to take a look as well

This revision is now accepted and ready to land.Feb 19 2019, 9:17 AM

Summary clusters, nodes and edges are retrieved from DenseMap with arbitrary order, which doesn't make result DOT file invalid.
Reverse iteration just exposes test issues.

Does that mean if we change the DenseMap hash, the test output will change? That seems pretty unfortunate. Can you sort the edges/nodes/etc. somehow to avoid this (either before or after dumping)?

Can you sort the edges/nodes/etc. somehow to avoid this

It's possible but what for? Just for test case?

Summary clusters, nodes and edges are retrieved from DenseMap with arbitrary order, which doesn't make result DOT file invalid.
Reverse iteration just exposes test issues.

The dot file is not invalid, but the output is sensitive to "arbitrary order" as you mentioned: this is not desirable. We should strive toward canonicalizing toward an order independent of the container properties.
So instead of iterating a DenseMap and emitting the output, there should be another step that sort the nodes in a deterministic way.

Actually this is already implemented for the bitcode emission I believe, even if the bitcode wouldn't be invalid either. The same logic should also be done for the DOT emission I believe.

evgeny777 requested changes to this revision.Feb 19 2019, 9:54 AM

The same logic should also be done for the DOT emission I believe.

Ok, I'll make a patch

This revision now requires changes to proceed.Feb 19 2019, 9:54 AM
Hahnfeld abandoned this revision.Feb 26 2019, 6:09 AM

Closing after D58631 landed which fixes both the bot and passes in my local build. Thanks @evgeny777!