AMDGPU: Reduce the number of expensive calls in SIFormMemoryClause

[cfang]

Summary

RPTracker::reset(MI) is a very expensive call when the number of virtual registers is huge.
We observed a long compilation time issue when RPT::reset() is called once for each cluster.

In this work, we call RPT.reset() only at the first seen cluster, and use advance() to get
the register pressure for the later clusters in the same basic block. This could effectively reduce the number
of the expensive calls and thus reduce the compile time.

Note: I am still seeing a couple LIT failures with the current state of this patch.

[rampitec]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

332

I am not sure, but likely RPT.getNext().isValid() has the same result as using !InitializedInBlock.

337

This second advance is not needed. reset() sets iterator before MI, not after.

340

LiveRegSet LiveRegs = ...
Make sure you are actually copying it, not taking a reference.

363

Move it down after "Ind->insertMachineInstrInMaps(*B);" and reset on B, not on MI. Technically MI's iterator is not valid for RPT purposes after bundling.

[cfang]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

340

How to clone a copy? I can not find a suitable RPT function for that purpose.

363

Shouldn't we also restore before the continue when there is just one instruction?

[cfang]

Make a few changes based on comments, Thanks!

1. Use RPT.getNext().isValid() instead of InitializedInBlock;
2. Make an actual copy of the Live Register Set in stead of the reference itself;
3. Move the restore of state down after "Ind->insertMachineInstrInMaps(*B);" and reset on B, not on MI

PS. There are still LIT test failures complaining instructions not matching.

[rampitec]

This looks good to me, be we need to understand what are these lit failures.

[cfang]

Fix LIT failures.

[rampitec]

LGTM. Thanks!

[MaskRay]

In your commit the message has just Reviewers:. The Reviewers: list does not necessarily mean all the people on the list have acknowledged the patch so Reviewers: is mostly useless. Many people agree that both Reviewed by: & Differential Revision: should be present.

arc amend can fetch the Phabricator summary and amend the local description.

You can install llvm/.git/hooks/pre-push to prevent accidental Summary:, Reviewers:, Subscribers: and Tags:.

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

   bool Changed = false;
 
   MaxVGPRs = TRI->getAllocatableSet(MF, &AMDGPU::VGPR_32RegClass).count();
   MaxSGPRs = TRI->getAllocatableSet(MF, &AMDGPU::SGPR_32RegClass).count();
   unsigned FuncMaxClause = AMDGPU::getIntegerAttribute(
       MF.getFunction(), "amdgpu-max-memory-clause", MaxClause);
 
   for (MachineBasicBlock &MBB : MF) {
+    GCNDownwardRPTracker RPT(*LIS);
     MachineBasicBlock::instr_iterator Next;
     for (auto I = MBB.instr_begin(), E = MBB.instr_end(); I != E; I = Next) {
       MachineInstr &MI = *I;
       Next = std::next(I);
 
       bool IsVMEM = isVMEMClauseInst(MI);
 
       if (!isValidClauseInst(MI, IsVMEM))
         continue;
 
-      RegUse Defs, Uses;
-      GCNDownwardRPTracker RPT(*LIS);
-      RPT.reset(MI);
+      if (!RPT.getNext().isValid())
+        RPT.reset(MI);
+      else { // Advance the state to the current MI.
+        RPT.advance(MachineBasicBlock::const_iterator(MI));
+        RPT.advanceBeforeNext();
+      }
 
-      if (!processRegUses(MI, Defs, Uses, RPT))
+      const GCNRPTracker::LiveRegSet LiveRegsCopy(RPT.getLiveRegs());
+      RegUse Defs, Uses;
+      if (!processRegUses(MI, Defs, Uses, RPT)) {
+        RPT.reset(MI, &LiveRegsCopy);
         continue;
+      }
 
       unsigned Length = 1;
       for ( ; Next != E && Length < FuncMaxClause; ++Next) {
         if (!isValidClauseInst(*Next, IsVMEM))
           break;
 
         // A load from pointer which was loaded inside the same bundle is an
         // impossible clause because we will need to write and read the same
         // register inside. In this case processRegUses will return false.
         if (!processRegUses(*Next, Defs, Uses, RPT))
           break;
 
         ++Length;
       }
-      if (Length < 2)
+      if (Length < 2) {
+        RPT.reset(MI, &LiveRegsCopy);
         continue;
+      }
 
       Changed = true;
       MFI->limitOccupancy(LastRecordedOccupancy);
 
       auto B = BuildMI(MBB, I, DebugLoc(), TII->get(TargetOpcode::BUNDLE));
       Ind->insertMachineInstrInMaps(*B);
 
+      // Restore the state after processing the bundle.
+      RPT.reset(*B, &LiveRegsCopy);
+
       for (auto BI = I; BI != Next; ++BI) {
         BI->bundleWithPred();
         Ind->removeSingleMachineInstrFromMaps(*BI);
 
         for (MachineOperand &MO : BI->defs())
           if (MO.readsReg())
             MO.setIsInternalRead(true);
       }