This is an archive of the discontinued LLVM Phabricator instance.

[LoopVersioning] Require loop-simplify form for loop versioning.
ClosedPublic

Authored by fhahn on Dec 6 2016, 10:13 AM.

Details

Summary

Requiring loop-simplify form for loop versioning ensures that the
runtime check block always dominates the exit block.

This patch closes #30958 (https://llvm.org/bugs/show_bug.cgi?id=30958).

Diff Detail

Event Timeline

fhahn updated this revision to Diff 80442.Dec 6 2016, 10:13 AM
fhahn retitled this revision from to [LoopVersioning] Check if exit block dominates runtime check block..
fhahn updated this object.
fhahn added reviewers: anemet, silviu.baranga.
fhahn added a subscriber: llvm-commits.
anemet accepted this revision.Dec 6 2016, 11:08 AM
anemet added a reviewer: hfinkel.
anemet added a subscriber: hfinkel.

LGTM. @hfinkel, is there a better way to handle infinite loops like this?

This revision is now accepted and ready to land.Dec 6 2016, 11:08 AM

Why are we trying to optimize loops which aren't in loop-simplify form?

fhahn added a comment.Dec 6 2016, 2:27 PM

@efriedma Only versioning loops that are in loop simplify form would prevent the error case. If that's the preferred solution I'd be happy to update the patch.

Making versionLoop() assert that the loop is in loop-simplify form is probably a good idea; versionLoop() is already assuming the loop has a preheader, so it's not much of a stretch. Granted, I'm not sure how much code needs to be fixed to correctly preserve loop-simplify form; versionLoop() itself doesn't preserve it correctly.

hfinkel edited edge metadata.Dec 7 2016, 7:27 AM

LGTM. @hfinkel, is there a better way to handle infinite loops like this?

I agree with Eli; we generally avoid this kind of situation by only handling loops that can be in loop-simplify form.

anemet added a comment.Dec 7 2016, 9:09 AM

LGTM. @hfinkel, is there a better way to handle infinite loops like this?

I agree with Eli; we generally avoid this kind of situation by only handling loops that can be in loop-simplify form.

Yeah, same here, thank Eli! I even tried -loop-simplify on the test case before first replying but I failed to notice that it created an exit block that *no longer* dominated the loop header. So I wrongly concluded that loop-simplify wouldn't help here.

fhahn updated this revision to Diff 81021.Dec 11 2016, 6:38 AM
fhahn retitled this revision from [LoopVersioning] Check if exit block dominates runtime check block. to [LoopVersioning] Require loop-simplify form for loop versioning..
fhahn updated this object.
fhahn edited edge metadata.

Require loop-simplify form for loop versioning, as per review comments.

fhahn added a comment.Dec 17 2016, 3:03 PM

This patch changed substantially since @anemet accepted it and it would be great if somebody could have another look.

The patch now requires loop-simplify form for loop versioning and the other optimizations requiring loop versioning, which may be to strict.

It’s good to avoid loop which are not in loop-simplify form.

This looks OK to me except a minor comment.

lib/Transforms/Scalar/LoopVersioningLICM.cpp
454 ↗(On Diff #81021)

Please move this check to "legalLoopStructure".

fhahn updated this revision to Diff 81941.Dec 19 2016, 5:38 AM

@ashutosh.nema thanks for pointing that out, I've moved the check to legalLoopStructure and removed the redundant checks.

ashutosh.nema accepted this revision.Dec 19 2016, 7:20 AM
ashutosh.nema added a reviewer: ashutosh.nema.

LGTM, thanks!

fhahn closed this revision.Dec 19 2016, 9:24 AM