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

[James Molloy via Phabricator via llvm-commits]

jmolloy added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:672
+    /// update with no users being pipelined.
+    virtual bool shouldIgnoreForPipelining(const MachineInstr *MI) const = 0;
+
----------------
jsji wrote:
> What is the use of this API? Did not see it in-tree.
There are only two targets in-tree that use the pipeliner (Hexagon and PPC9). Both of these use dedicated hardware loop instructions so there is no induction variable update. However, the more traditional loop code sequence:

  %newindvar = ADDri %indvar, 1
  %done = CMPri %newindvar, TripCount
  BR %done

we have two non-terminator instructions that *must not be pipelined*, because there is no guarantee that they will end up in stage 0 (which is the only valid stage for them). Tanya Lattner's thesis (and the original SMS paper) states that they strip canonical indvar updates while pipelining and add them in later when constructing the pipelined loop. This function allows the target to specify these instructions to strip.


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


================
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;
----------------
jsji wrote:
> I don't know why we would like to do two things in one API? Can we split?
> 
Good idea!


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:693
+    /// should be removed.
+    virtual void disposed() = 0;
+  };
----------------
jsji wrote:
> 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?
I agree. This is a hook point to allow PPC and Hexagon to remove the LOOP setup instruction in the preheader. I don't particularly want to bake that implementation detail into this target-independent header though.

I've reworded this to remove references to preheaders and just mention that the loop will be removed and once this function is called no other functions can be called. Does this look alright?


================
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
----------------
jsji wrote:
> I don't quite follow when we should call *splice* to `NewPreheader` for a target, and why Hexagon needs it, but PowerPC don't? 
Hexagon wants the LOOP instruction in the immediate predecessor of the pipelined loop kernel:

  prolog1:
  prolog2:
    LOOP
  kernel:
    ENDLOOP

Whereas PPC wants it to stay in the preheader to the prolog, so it can use BDZ within the prolog to adapt the loop count:

    LOOP
  prolog1:
    BDZ
  prolog2:
    BDZ
  kernel:
    BDNZ


================
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()) {
----------------
jsji wrote:
> I think we skip this loop intentionally when enabling PowerPC target , why we need it back?
We needed it because I was calling analyzeLoop after doing some modifications to the loop. I've changed so that ModuloScheduleExpander calls analyzeLoop early, so the preheader is in the right place and we don't need this change any more. Thanks for noticing this!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67167