Page MenuHomePhabricator

[StaticAnalyzer] LoopUnrolling fixes
ClosedPublic

Authored by szepet on Aug 24 2017, 4:02 AM.

Details

Summary
  1. The LoopUnrolling feature needs the LoopExit included in the CFG so added this dependency via the config options
  2. The LoopExit element can be encountered even if we haven't encountered the block of the corresponding LoopStmt. So the asserts were not right.
  3. If we are caching out the Node then we get a nullptr from generateNode which case was not handled.

Diff Detail

Repository
rL LLVM

Event Timeline

szepet created this revision.Aug 24 2017, 4:02 AM
NoQ edited edge metadata.Aug 24 2017, 7:30 AM

Yeah, looks good.

Some tests would be great, at least for 2 (in what case do we not have the loop on the stack?)

szepet updated this revision to Diff 112713.Aug 25 2017, 10:30 AM

Test added.

zaks.anna added inline comments.Aug 27 2017, 12:54 AM
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
190 ↗(On Diff #112713)

I would rather keep this method as is and add the extra logic elsewhere (ex: the place where includeLoopExitInCFG is used). To me, AnalyzerOptions::includeLoopExitInCFG() returns the value of the corresponding parameter and I would not expect it to use anything else.

test/Analysis/loop-unrolling.cpp
276 ↗(On Diff #112713)

nit: "loop" "exit" and "loop" "stack" are separate words, so consider using "_" to separate them.

szepet updated this revision to Diff 112824.Aug 27 2017, 6:43 AM

Update based on comments.

szepet marked 2 inline comments as done.Aug 27 2017, 6:44 AM
NoQ accepted this revision.Aug 28 2017, 1:02 AM

LGTM!

This revision is now accepted and ready to land.Aug 28 2017, 1:02 AM
This revision was automatically updated to reflect the committed changes.