[PATCH] D71821: [LoopFusion] Move instructions from FC1.Preheader to FC0.Preheader when proven safe.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some minor comments only.



================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:89
 STATISTIC(NonAdjacent, "Loops are not adjacent");
-STATISTIC(NonEmptyPreheader, "Loop has a non-empty preheader");
+STATISTIC(UnsafePreheader, "Loop has an unsafe preheader");
 STATISTIC(FusionNotBeneficial, "Fusion is not beneficial");
----------------
`unsafe preheader` is probably not the best term. Maybe spell it out: `non-empty preheader with instructions that cannot be moved`?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:485
+    I.moveBefore(MovePos);
+  }
+}
----------------
`while (FromBB.size() > 1)` might be easier, with `*I = FromBB.front()`.

Why do you take the DT, PDT, DI? I would not or just them to `assert(isSafe..)`


================
Comment at: llvm/test/Transforms/LoopFusion/diagnostics_missed.ll:219
 attributes #0 = { nounwind readnone speculatable willreturn }
+attributes #1 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="all" "less-precise-fpmad"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="ppc64le" "target-features"="+altivec,+bpermd,+crypto,+direct-move,+extdiv,+htm,+power8-vector,+vsx,-power9-vector,-qpx,-spe" "unsafe-fp-math"="false" "use-soft-float"="false" }
 
----------------
Delete the attributes please.


================
Comment at: llvm/test/Transforms/LoopFusion/simple.ll:335
+for.first.exit:
+  %add = add nsw i32 0, 1
+  br label %for.second
----------------
Maybe make it more realistic by adding an argument `%x` and `1`, as your commit message example shows. We can then also have a negative test where you replace `1` with `%i` from the first loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71821