[PATCH] D67167: [MachinePipeliner] Improve the TargetInstrInfo API analyzeLoop/reduceLoopCount

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.
Herald added a subscriber: wuzish.

General idea is great, some comments when I looked at PowerPC part. Thanks.



================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:665
+  /// Object returned by analyzeLoopForPipelining. Allows software pipelining
+  /// implementations to query attributes of the loop being pipelined.
+  class PipelinerLoopInfo {
----------------
No just query, but also modify the loop?


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:672
+    /// update with no users being pipelined.
+    virtual bool shouldIgnoreForPipelining(const MachineInstr *MI) const = 0;
+
----------------
What is the use of this API? Did not see it in-tree.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:682
+    virtual Optional<bool>
+    getTripCountGreaterCondition(int TC, MachineBasicBlock &MBB,
+                                 SmallVectorImpl<MachineOperand> &Cond) = 0;
----------------
I might be wrong, but it reads like a `get accessor `, so looks like *READ* only, but we actually `create` things with API.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:688
+    /// Additionally the loop's preheader is now NewPreheader.
+    virtual void adjust(int TripCountAdjust,
+                        MachineBasicBlock *NewPreheader) = 0;
----------------
I don't know why we would like to do two things in one API? Can we split?



================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:693
+    /// should be removed.
+    virtual void disposed() = 0;
+  };
----------------
It might be a little confusing to call `LoopInfo->adjust` after calling `LoopInfo->disposed`.
So maybe we should be more specific about what we dispose here in API name?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3980
+
+  void adjust(int TripCountAdjust, MachineBasicBlock *NewPreheader) override {
+    // If the loop trip count is a compile-time value, then just change the
----------------
I don't quite follow when we should call *splice* to `NewPreheader` for a target, and why Hexagon needs it, but PowerPC don't? 


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:4027
 
+  // The loop set-up instruction will be in a predecessor block
+  for (MachineBasicBlock *PB : PreHeader.predecessors()) {
----------------
I think we skip this loop intentionally when enabling PowerPC target , why we need it back?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67167