[PATCH] D149331: [LCSSA] Don't invalidate entire loop in SCEV

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

@vporpo Thanks for the report! I've reverted the problematic changes in https://github.com/llvm/llvm-project/commit/143ed21b26b2c68695e9f74f0ce4632b8a2e000b and https://github.com/llvm/llvm-project/commit/3c9cf023db32ba2cfa1e052ddc58f57dd080995c and have added a test in https://github.com/llvm/llvm-project/commit/79115aebb759eb1ef8cdcc35f228d88f70c7bfc2.

In this test case, the BECount of the second loop depends on `%ld` defined in the first loop. LCSSA construction does not change this, as SCEV looks through LCSSA phi nodes.

However, when we peel the loop, the exit value is now a phi node between the value `%ld` from the loop, and the `%ld.peel` clone from the peeled iteration. Directly using the SCEV node for `%ld` in the BECount of the second loop is no longer valid.

Loop peeling tries to handle this by invalidating the (former LCSSA, now real) phi nodes in the exits. This worked previously, because the BECount calculation for the second loop would go through that LCSSA phi node and cache it's value in the ValueToExpr map. This means that if we invalidate the phi node, we would also forget memoized values for the corresponding expression, which includes the BECount for the second loop. However, without the invalidation during LCSSA construction, there will be no value cached for the phi node, so we also won't invalidate the corresponding expression.

It's a bit hard to determine who is at fault here, because both LCSSA and peeling do pretty reasonable things. I think maybe the best fix for this issue would be to make LCSSA not invalidate the value, but rather explicitly create the SCEV for the new LCSSA phi node, if one exists for the instruction in the loop. @fhahn Do you have any thoughts on this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149331/new/

https://reviews.llvm.org/D149331