[PATCH] D142255: [WIP] Loop peeling opportunity for identity operators

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

The approach looks good to me. Got some smaller remarks, nothing critical since it doesn't affect correctness.



================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:471-487
+// When a loop contains an accumulator, an initial identity operation
+// may be avoided by peeling a single time.  For example, consider this loop
+// int Total = 0;
+// int Product = 1;
+// for (I = 0; I < 100000; ++I) {
+//   Total += foo(I);
+//   Product *= bar(I);
----------------
Consider using doxygen comments (starting with `///`). Doxygen code samples are [[ https://www.doxygen.nl/manual/markdown.html#md_codeblock | indented by 4 spaces ]].

Could you explicitly list all the cases that should be handled. E.g.

 * Integer, initial value 0, add
 * Integer, initial value 1, mul
 * Float, initial value 0, fadd
 * Float, initial value 0, fmadd
 * Float, initial value 1, fmul
 * ...


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:506
+    // Return the incoming constant value from \p BB or nullptr
+    auto constIn = [&](BasicBlock *BB, bool) -> Constant * {
+      if (L.contains(BB))
----------------
[style] Functions start with a verb, e.g. `getConstIn`


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:516
+      if (ConstantFP *CFP = dyn_cast<ConstantFP>(C))
+        return CFP->isZeroValue() && any_of(Phi.users(), [&](User *U) -> bool {
+                 if (beneficial(*U, Phi, Instruction::FAdd))
----------------
`ConstantFP::isZeroValue()` returns true for negative and positive zero; should we do this only with `-ffast-math` (or the subflag that controls this)?


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:529
+        if (CI->isZeroValue())
+          Op = (CI->getBitWidth() == 1) ? Instruction::Or : Instruction::Add;
+        else if (CI->isOneValue())
----------------
Would both (`or` and `add`; respectively `mul` and `and` with `isAllOnes()`) be valid independent of bit with. E.g. The PHI value might be a flags enum so we'd do a bitwise operation.

```
flags_t Flags = 0;
for (I = 0; I < 100000; ++I) {
   Total |= enable_flag(I);
}
```


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:544
+  // Peeling may be beneficial for a use in the loop with operator \p Op.
+  bool beneficial(const User &U, const PHINode &Phi,
+                  const BinaryOperator::BinaryOps Op) const {
----------------
[style] Functions start with a verb


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:558
+                  SE.getSmallConstantMaxTripCount(&L) *
+                          Phi.getType()->getScalarSizeInBits() % 128 !=
+                      0);
----------------
128 is is really big; x86 is 2, 4, 8 or 16 elements. Is TTI to get native vector size?


================
Comment at: llvm/test/Transforms/LoopUnroll/peel-loop-identity-op.ll:4
+
+declare i32 @g()
+
----------------
Consider adding a description to the test


================
Comment at: llvm/test/Transforms/LoopUnroll/peel-loop-identity-op.ll:7
+define i32 @f() {
+; Preheader:
+; CHECK-LABEL: @f(
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142255