Page MenuHomePhabricator

LoopDistribute/LAA: Add tests to catch regressions

Authored by arsenm on Jun 7 2019, 4:59 PM.



I broke 2 of these with a patch, but were not covered by existing tests.

Diff Detail

Event Timeline

arsenm created this revision.Jun 7 2019, 4:59 PM
arsenm updated this revision to Diff 203686.Jun 8 2019, 6:43 AM

Catch another broken case

As I understand the language reference, ("The optimizers may change the order of volatile operations relative to non-volatile operations.") the loop can be distributed as long as there all volatile operations stay in the same loop. I this is meant to test the current implementation, can you add a comment to make this clear (such as "TODO: distribution of volatile may be possible under some circumstance, but the current implementation does not touch them")


This might not sufficiently check that no distribution occured. The br i1 %exitcond does match br i1 %exitcond.ldist1. Is there a guaranteed that the ldist loops are added before the original %for.body/%exitcond pair?


[nit] To avoid some ambiguity understanding the sentence: "Make sure the loop is not vectorized due to the volatile, even though it has no memory writes"


Add comment about what this is supposed to test? Such as "Ensure that volatile stores are not vectorized"

arsenm updated this revision to Diff 203900.Jun 10 2019, 2:34 PM
arsenm marked 2 inline comments as done.

Address comments, move one test to loop vectorizer

Meinersbur accepted this revision.Jun 11 2019, 4:00 PM
Meinersbur added inline comments.

What I meant was adding the TODO here, to avoid the impression that the "no distribution" is a correctness requirement.

This revision is now accepted and ready to land.Jun 11 2019, 4:00 PM
arsenm closed this revision.Jun 12 2019, 6:12 AM
arsenm marked an inline comment as done.