[PATCH] D105723: [LSR] Do not hoist IV if it is not post increment case. PR43678

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi,

I am not sure I understand the issue here.
Could you paste the IR with the wrong transformation (before this patch) for one of the test (e.g., the smaller one)?

Some high-level comments:

- You can put more than one function in a test if you want to avoid to create two different files?
- Could you change the filenames of the tests with something more human friendly so that we know what is tested just looking at the filename? (You can put the PR number in the comment in the test)

Cheers,
-Quentin



================
Comment at: llvm/test/Transforms/LoopStrengthReduce/pr43678-2.ll:4
+
+; Check that LSR produce correct IR in terms of SSA.
+; CHECK-LABEL: test
----------------
Could you be more explicit about what this test is exercising?

E.g., what IV causes the problem and how it was wrongly transformed (or what it looks like to generate something correct.)


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/pr43678-2.ll:5
+; Check that LSR produce correct IR in terms of SSA.
+; CHECK-LABEL: test
+define void @test() {
----------------
Could you add more check lines?

As is, we only check that we don't crash.

You can use `update_test_checks.py`


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/pr43678.ll:5
+
+; Check that LSR produce correct IR in terms of SSA.
+; CHECK-LABEL: test
----------------
Same comments about the check lines and the explanation of what is being tested here.


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

https://reviews.llvm.org/D105723