[PATCH] D22630: Loop rotation

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Aditya,

Thanks updating the patch! I have several remarks regarding tests (I hope to get to reviewing the actual code soon as well), please find them inline.

Thanks,
Michael


================
Comment at: llvm/test/Analysis/ScalarEvolution/2012-03-26-LoadConstant.ll:1
@@ -1,1 +1,2 @@
-; RUN: opt < %s -basicaa -globalopt -instcombine -loop-rotate -licm -instcombine -indvars -loop-deletion -constmerge -S | FileCheck %s
+; RUN: opt < %s -basicaa -globalopt -instcombine -loop-rotate -licm -simplifycfg -S | FileCheck %s
+
----------------
Why is this change needed?

================
Comment at: llvm/test/Transforms/LoopRotate/indirectbr-1.ll:10
@@ +9,3 @@
+
+ at f.x = internal global [3 x i8*] [i8* blockaddress(@f, %F), i8* blockaddress(@f, %G), i8* blockaddress(@f, %H)], align 16
+
----------------
I'm pretty sure this test can be reduced further. Are you still able to reproduce the failure it's intended to catch? If so, you can try using `bugpoint` again as it's been improved recently. If it doesn't help, it still might be useful to understand why the test causes crashes and thus understand what IR is actually important and what can be safely removed.

================
Comment at: llvm/test/Transforms/LoopRotate/indirectbr-1.ll:77-78
@@ +76,4 @@
+
+attributes #0 = { nounwind uwtable "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
----------------
E.g. these attributes can definitely be removed.

================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate-0.ll:1-3
@@ +1,4 @@
+; RUN: opt -S < %s -loop-rotate -verify-dom-info -verify-loop-info | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
Can we combine these tests into one file (it'll also be good to use the same naming scheme for functions/blocks/variables if possible)?

================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate-2.ll:1
@@ +1,2 @@
+; RUN: opt -S < %s -loop-rotate -verify-dom-info -verify-loop-info | FileCheck %s
+
----------------
This test look very similar to `loop-rotate-0.ll` (and some other tests actually). While I understand, that they can exercise slightly different parts of implementation and crash in a different ways, without a proper comment it looks redundant (actually, I suspect it is redundant). Tests clean-up that I'm asking for should help revealing if they test the same case, or not - that's why I think it's important.

================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate-4.ll:55-63
@@ +54,11 @@
+  switch i32 undef, label %if.else427 [
+    i32 26, label %if.then130
+    i32 24, label %if.then130
+    i32 23, label %if.then130
+    i32 22, label %if.then130
+    i32 20, label %if.then130
+    i32 19, label %if.then130
+    i32 18, label %if.then130
+    i32 12, label %if.then130
+    i32 6, label %if.then149
+    i32 1, label %if.then172
----------------
Even if bugpoint fails to reduce this test further, you should be able to manually remove unneeded successors of the switch, and then replace it with a branch (it's worth trying the latest bugpoing - probably it'll reduce it). That should make the test much smaller and easier to understand.


https://reviews.llvm.org/D22630