[PATCH] D82927: Intergerate Loop Peeling into Loop Fusion

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Could you update the patch with context (`git diff -U999999`)?



================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:704
+  std::pair<bool, Optional<unsigned>>
+  HaveIdenticalTripCounts(const FusionCandidate &FC0,
+                          const FusionCandidate &FC1) const {
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | Function names should start with a lower case letter. ]]


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:712
       LLVM_DEBUG(dbgs() << "Trip count of first loop could not be computed!");
-      return false;
+      return Res;
     }
----------------
Is it possible to `return {false, None}`? The pair type should be deducible by the compiler and it immediately shows that it returns.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:727
 
-    return (TripCount0 == TripCount1);
+    Res.first = (TripCount0 == TripCount1);
+
----------------
Did you consider assigning it to a variable with a meaningful name first such as `SameTripCount`, then `return {SameTripCount, diff}`?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:749
+
+    int diff = TC0 - TC1;
+
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | Local variable name should start with a capital letter. ]]


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:766
+
+  void PeelFusionCandidate(FusionCandidate &FC0, const FusionCandidate &FC1,
+                           unsigned PeelCount) {
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | Function names should start with a lower case letter ]]


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:777
+
+      auto IdenticalTripCount = HaveIdenticalTripCounts(FC0, FC1);
+
----------------
The call result is only used when asserts are enabled, please use `#ifndef NDEBUG` around the call.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:790
+      // In this case the iterations of the loop are constant, so the first
+      // loop will exectue completely (will not jump from one of
+      // the peeled blocks to the second loop). Here we are updating the
----------------
[typo] exec**tu**e


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:953-955
+          // All analysis has completed and has determined that fusion is
+          // legal and profitable. At this point, start transforming the
+          // code and perform fusion.
----------------
[nit] Please avoid such spurious changes in the Differential


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:166
              "optimizations"));
+
 static cl::opt<unsigned>
----------------
[nit] Please avoid unrelated changes


================
Comment at: llvm/lib/Transforms/Utils/CMakeLists.txt:38
   Local.cpp
+  LoopPeel.cpp
   LoopRotationUtils.cpp
----------------
Did you consider making this rename a separate patch? It adds a lot of noise to this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82927