[PATCH] D60645: [LSR] PR41445: Rewrite misses some fixup locations if it splits critical edge.

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

The source change looks good to me, but I would like a bit more work on the test itself.

Comments below.



================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:5359
+
+      // PR41445: If LSR split critical edge and phi node has other pending
+      // fixup operands, we need to update those pending fixups. Otherwise
----------------
We don't usually put PR number in comments. Just make sure to put it in your commit message.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:5381
+              // Otherwise it might be moved to another PHI and requires update.
+              if (!foundInOriginalPHI) {
+                // If not found in original PHI we search in incoming blocks.
----------------
To reduce indentation, use:
```
if (foundOriginalPHI)
  continue;
```


================
Comment at: test/Transforms/LoopStrengthReduce/pr41445.ll:1
+; RUN: opt -S -loop-reduce < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
Use a more descriptive filename please: e.g., missing-phi-operand-update.


================
Comment at: test/Transforms/LoopStrengthReduce/pr41445.ll:1
+; RUN: opt -S -loop-reduce < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
qcolombet wrote:
> Use a more descriptive filename please: e.g., missing-phi-operand-update.
Please run `opt -instnamer` to get rid of the implicit variable.


================
Comment at: test/Transforms/LoopStrengthReduce/pr41445.ll:2
+; RUN: opt -S -loop-reduce < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
----------------
Put the PR number in comments in the test.


================
Comment at: test/Transforms/LoopStrengthReduce/pr41445.ll:5
+
+define i32 @foo(i32* %A, i32 %t) {
+entry:
----------------
Add a comment describing the characteristic of the test.


================
Comment at: test/Transforms/LoopStrengthReduce/pr41445.ll:6
+define i32 @foo(i32* %A, i32 %t) {
+entry:
+  br label %loop.32
----------------
Could you add a test that fall into `foundInOriginalPHI == true` if that's not already the case?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60645