Page MenuHomePhabricator

LoopDistribute/LAA: Add tests to catch regressions
ClosedPublic

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

Details

Reviewers
Meinersbur
Summary

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")

test/Transforms/LoopDistribute/basic-with-memchecks.ll
120–125

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?

173–174

[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"

test/Transforms/LoopVectorize/write-only.ll
27

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.
test/Transforms/LoopDistribute/basic-with-memchecks.ll
114

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.

r363158