This is an archive of the discontinued LLVM Phabricator instance.

[LIR] Fix mis-compilation with unwinding
ClosedPublic

Authored by haicheng on May 25 2016, 12:58 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

haicheng updated this revision to Diff 58478.May 25 2016, 12:58 PM
haicheng retitled this revision from to [LIR] Fix mis-compilation with unwinding.
haicheng updated this object.
haicheng added reviewers: mcrosier, mssimpso.
haicheng set the repository for this revision to rL LLVM.
mcrosier edited edge metadata.May 25 2016, 1:57 PM

Looks conservatively correct and GTM. What do you think @eli.friedman?

majnemer requested changes to this revision.May 25 2016, 2:13 PM
majnemer added a reviewer: majnemer.

I'm pretty sure this is wrong. It only considers calls in the same basic block as the store. However, the store could post-dominate a call to a function which never returns.

Instead, I suggest using llvm::isGuaranteedToTransferExecutionToSuccessor instead of the current, wrong, heuristic of "does this BB dominate all the exit blocks".

Due to the nature of llvm::isGuaranteedToTransferExecutionToSuccessor, I suggest that it's logic be factored out so that you can cache some of the intermediate results.

test/Transforms/LoopIdiom/unwind.ll
4–50 ↗(On Diff #58478)

There is no need to provide the definition of f. You could use my reduced testcase, I'm pretty sure it is minimal.

This revision now requires changes to proceed.May 25 2016, 2:13 PM
haicheng added inline comments.May 25 2016, 2:26 PM
test/Transforms/LoopIdiom/unwind.ll
4–50 ↗(On Diff #58478)

Targets like AArch64 do not generate attribute uwtable so that I include the definition of f.

majnemer added inline comments.May 25 2016, 2:28 PM
test/Transforms/LoopIdiom/unwind.ll
4–50 ↗(On Diff #58478)

Er, this testcase has an explicit triple. What does AArch64 have to do with anything?

eli.friedman added inline comments.May 26 2016, 10:48 AM
lib/Transforms/Scalar/LoopIdiomRecognize.cpp
392 ↗(On Diff #58478)

This check is in collectStores, which is run once per basic block... so this won't notice a call which throws in a different basic block ("for (...) { p[i] = 0; if (a) throwing_fn(); }"),

test/Transforms/LoopIdiom/unwind.ll
18 ↗(On Diff #58478)

No reason to include the implementation of f in the testcase.

mcrosier resigned from this revision.Jun 7 2016, 6:59 AM
mcrosier removed a reviewer: mcrosier.
mcrosier removed a subscriber: mcrosier.

Are you planning to continue working on this?

Yes, but not in a week or two. I am tied up with some other projects.

mssimpso resigned from this revision.Jun 22 2016, 1:53 PM
mssimpso removed a reviewer: mssimpso.

Resigning since David and Eli are reviewing.

haicheng updated this revision to Diff 62879.Jul 6 2016, 8:34 AM
haicheng edited edge metadata.
haicheng marked 5 inline comments as done.

Sorry for the long delay.

Just use computeLoopSafetyInfo (which calls isGuaranteedToTransferExecutionToSuccessor) to decide whether the loop may throw or not.

Please take a look.

The code looks fine. The comments could use a bit of refinement.

lib/Transforms/Scalar/LoopIdiomRecognize.cpp
220 ↗(On Diff #62879)

Useless comment.

223 ↗(On Diff #62879)

It would be useful to explain why this check is necessary. Among other things, it would be good to mention that existing transforms involve hoisting stores into the loop pre-header.

haicheng updated this revision to Diff 62919.Jul 6 2016, 11:23 AM
haicheng edited edge metadata.
haicheng marked 2 inline comments as done.

Refine the comments as suggested by Eli.

eli.friedman accepted this revision.Jul 6 2016, 11:37 AM
eli.friedman added a reviewer: eli.friedman.

LGTM.

This revision was automatically updated to reflect the committed changes.