[PATCH] D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable.

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:36
+
+/// Check wether speculating the users of a PHI node around the PHI
+/// will be safe.
----------------
wether --> whether


================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:67
+    // instructions and allow handling stores in easily proven safe cases.
+    if (mayBeMemoryDependent(*UI)) {
+      DEBUG(dbgs() << "  Unsafe: can't speculate use: " << *UI << "\n");
----------------
This is too limiting.  The scenario is commonly created by the store sinking optimization -- which involves memory operation.


================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:107
+        // we're done.
+        if (PotentialSpecSet.count(OpI))
+          continue;
----------------
why not checking this first?


================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:112
+        // If when we directly test whether this is safe it fails, bail.
+        if (UnsafeSet.count(OpI) || mayBeMemoryDependent(*OpI)) {
+          DEBUG(dbgs() << "  Unsafe: can't speculate transitive use: " << *OpI
----------------
What is the second check about? OpI is not a code motion candidate.

The first check seems wrong to me too.  At this point, we determined that OpI is not in a block that dominates the the target blocks of the code motion (hoisting), so it should return false immediately.  

I don't see the need to do a DFS walk to check the chains of operands either. 

The caching here seems also wrong and confusing.


================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:185
+
+    MatCost += TTI.getIntImmCost(IncomingC->getValue(), IncomingC->getType());
+    ++IncomingConstants[IncomingC];
----------------
This cost should be weighted by the block frequency of the constant materializing block.


================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:186
+    MatCost += TTI.getIntImmCost(IncomingC->getValue(), IncomingC->getType());
+    ++IncomingConstants[IncomingC];
+  }
----------------
This should remember the total frequency for incomingC


================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:256
+
+  DEBUG(dbgs() << "    Cost savings " << (MatCost - FoldedImmCost) << ": " << PN
+               << "\n");
----------------
The cost analysis here assumes non-speculative case where the uses of phi  are fully anticipated. Consider the real speculative case:


```

/// entry:
///     br i1 %flag, label %a, label %b
///
///   a:
///     br label %merge
///
///   b:
///     br label %merge
///
///   merge:
///     %p = phi i32 [ 7, %a ], [ 11, %b ]
///     br i1 %flag2, label %use, label %exit
///
///   use:
///     %sum = add i32 %arg, %p
///     br label %exit
///
///   exit:
///    %s = phi ([0, ...], [%sum, ..])
///     ret i32 %s

```
Hoisting the add into %a and %b is speculative. Additional speculation cost is involved depending on the cost of add and relative frequency of %a+%b vs %use.


consider another case with triagular cfg:

```

/// entry:
///     br i1 %flag, label %a, label %merge
///   a:
///     br label %merge
///
///   merge:
///     %p = phi i32 [ 7, %a ], [ 11, %entry ]
///     %sum = add i32 %arg, %p
///    ret i32 %sum

```

hoisting 'add' into entry and '%a' introduces runtime overhead.


================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:684
+                            [&](PHINode *PN) {
+                              return !isSafeAndProfitableToSpeculateAroundPHI(
+                                  *PN, CostSavingsMap, PotentialSpecSet,
----------------
Looks like the PotentialSpecSet, UnsafeSet is shared across different PN candidates -- this does not look correct.


https://reviews.llvm.org/D37467