[PATCH] D79787: [IndVarSimplify][LoopUtils] Avoid TOCTOU/ordering issues (PR45835)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: dmajor, reames, mkazantsev, fhahn, efriedma.
lebedev.ri added a project: LLVM.
Herald added subscribers: javed.absar, hiraditya.

Currently, `rewriteLoopExitValues()`'s logic is roughly as following:

> Loop over each incoming value in each PHI node.
>  Query whether the SCEV for that incoming value is high-cost.
>  Expand the SCEV.
>  Perform sanity check (`isValidRewrite()`, D51582 <https://reviews.llvm.org/D51582>)
>  Record the info
>  Afterwards, see if we can drop the loop given replacements.
>  Maybe perform replacements.

The problem is that we interleave SCEV cost checking and expansion.
This is A Problem, because `isHighCostExpansion()` takes special care
to not bill for the expansions that were already expanded, and we can reuse.

While it makes sense in general - if we know that we will expand some SCEV,
all the other SCEV's costs should account for that, which might cause
some of them to become non-high-cost too, and cause chain reaction.

But that isn't what we are doing here. We expand *all* SCEV's, unconditionally.
So every next SCEV's cost will be affected by the already-performed expansions
for previous SCEV's. Even if we are not planning on keeping
some of the expansions we performed.

Worse yet, this current "bonus" depends on the exact PHI node
incoming value processing order. This is completely wrong.

As an example of an issue, see @dmajor's `pr45835.ll` - if we happen to have
a PHI node with two(!) identical high-cost incoming values for the same basic blocks,
we would decide first time around that it is high-cost, expand it,
and immediately decide that it is not high-cost because we have an expansion
that we could reuse (because we expanded it right before, temporarily),
and replace the second incoming value but not the first one;
thus resulting in a broken PHI.

What we instead should do for now, is not perform any expansions
until after we've queried all the costs.

Later, in particular after `isValidRewrite()` is an assertion (D51582 <https://reviews.llvm.org/D51582>)
we could improve upon that, but in a more coherent fashion.

See PR45835 <https://bugs.llvm.org/show_bug.cgi?id=45835>


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79787

Files:
  llvm/lib/Transforms/Utils/LoopUtils.cpp
  llvm/test/Transforms/IndVarSimplify/pr45835.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D79787.263427.patch
Type: text/x-patch
Size: 6504 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200512/7bcbd1cb/attachment-0001.bin>