[AMDGPU] Disable the SIFormMemoryClauses pass at -O1

[bsaleil]

Summary

This patch disables the SIFormMemoryClauses pass at -O1. This pass has a significant impact on compilation time, so we only want it to be enabled starting from -O2.

[arsenm]

Can we address it being slow instead of just turning of everything that is? There's no reason it should be slow, but the way it uses liveness information is horrible

[bsaleil]

In D101939#2740008, @arsenm wrote:

Can we address it being slow instead of just turning of everything that is? There's no reason it should be slow, but the way it uses liveness information is horrible

I'm planning to disable it at -O1 for now, then investigate to improve the implementation for the default optimization level so we can eventually re-enable the pass at O1 too. Does that work for you?

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

1171

Needs a comment/fixme for why

[rampitec]

In D101939#2740008, @arsenm wrote:

Can we address it being slow instead of just turning of everything that is? There's no reason it should be slow, but the way it uses liveness information is horrible

Besides the speed I also do not think this pass belongs at -O1 at all. This not an essential trivial optimization.

[foad]

In D101939#2740008, @arsenm wrote:

Can we address it being slow instead of just turning of everything that is? There's no reason it should be slow, but the way it uses liveness information is horrible

On small functions I think the run time of this pass is dominated by the two calls to TRI->getAllocatableSet, which means two identical calls to SIRegisterInfo::getReservedRegs, which has a very large fixed cost. Is there some way the Reserved register set can be cached (in the SIMachineFunctionInfo object?)?

[bsaleil]

Add the TargetRegisterInfo::getAllocatableSets method to avoid calling getReservedRegs multiple times when we want allocatable sets for multiple register classes.

[bsaleil]

In D101939#2741529, @foad wrote:

In D101939#2740008, @arsenm wrote:

Can we address it being slow instead of just turning of everything that is? There's no reason it should be slow, but the way it uses liveness information is horrible

On small functions I think the run time of this pass is dominated by the two calls to TRI->getAllocatableSet, which means two identical calls to SIRegisterInfo::getReservedRegs, which has a very large fixed cost. Is there some way the Reserved register set can be cached (in the SIMachineFunctionInfo object?)?

@foad you're right. But caching the result of getReservedRegs is not easy because it difficult to detect that the calls are from consecutive calls to getAllocatableSet. I instead added the TargetRegisterInfo::getAllocatableSets method allowing to get allocatable sets for multiple register classes so we can call getReservedRegs a single time for all the classes.

[arsenm]

It's long been on my todo list to replace reserved registers with reserved reg units which should provide a similar speedup (and just generally be cleaner)

[arsenm]

In D101939#2744979, @arsenm wrote:

It's long been on my todo list to replace reserved registers with reserved reg units which should provide a similar speedup (and just generally be cleaner)

I think I'm trying to take a stab at this

[arsenm]

In D101939#2745028, @arsenm wrote:

In D101939#2744979, @arsenm wrote:

It's long been on my todo list to replace reserved registers with reserved reg units which should provide a similar speedup (and just generally be cleaner)

I think I'm trying to take a stab at this

I started looking at this, but I'm not sure it makes sense to do without also changing quite a bit of other infrastructure to work on regunits at the same time

llvm/lib/CodeGen/TargetRegisterInfo.cpp

270 ↗

(On Diff #343689)

Why is this recomputing the reserved regs instead of just querying MRI::getReservedRegs?

276 ↗

(On Diff #343689)

This is a separate patch

280–289 ↗

(On Diff #343689)

Most of this function is a duplictae of getAllocatableSet

[bsaleil]

Revert changes improving the pass implementation, these changes will be addressed in another patch.

[foad]

llvm/lib/CodeGen/TargetRegisterInfo.cpp

270 ↗

(On Diff #343689)

D102318

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

 void GCNPassConfig::addOptimizedRegAlloc() {
   // Allow the scheduler to run before SIWholeQuadMode inserts exec manipulation
   // instructions that cause scheduling barriers.
   insertPass(&MachineSchedulerID, &SIWholeQuadModeID);
   insertPass(&MachineSchedulerID, &SIPreAllocateWWMRegsID);
 
   if (OptExecMaskPreRA)
     insertPass(&MachineSchedulerID, &SIOptimizeExecMaskingPreRAID);
+
+  // This is not an essential optimization and it has a noticeable impact on
+  // compilation time, so we only enable it from O2.
+  if (TM->getOptLevel() > CodeGenOpt::Less)
     insertPass(&MachineSchedulerID, &SIFormMemoryClausesID);
 
   // This must be run immediately after phi elimination and before
   // TwoAddressInstructions, otherwise the processing of the tied operand of
   // SI_ELSE will introduce a copy of the tied operand source after the else.
   insertPass(&PHIEliminationID, &SILowerControlFlowID, false);
 
   if (EnableDCEInRA)
     insertPass(&DetectDeadLanesID, &DeadMachineInstructionElimID);

llvm/test/CodeGen/AMDGPU/llc-pipeline.ll

 ; GCN-O1-NEXT:         Rename Disconnected Subregister Components
 ; GCN-O1-NEXT:         Machine Instruction Scheduler
 ; GCN-O1-NEXT:         MachinePostDominator Tree Construction
 ; GCN-O1-NEXT:         SI Whole Quad Mode
 ; GCN-O1-NEXT:         Virtual Register Map
 ; GCN-O1-NEXT:         Live Register Matrix
 ; GCN-O1-NEXT:         SI Pre-allocate WWM Registers
 ; GCN-O1-NEXT:         SI optimize exec mask operations pre-RA
-; GCN-O1-NEXT:         SI Form memory clauses
 ; GCN-O1-NEXT:         Machine Natural Loop Construction
 ; GCN-O1-NEXT:         Machine Block Frequency Analysis
 ; GCN-O1-NEXT:         Debug Variable Analysis
 ; GCN-O1-NEXT:         Live Stack Slot Analysis
 ; GCN-O1-NEXT:         Virtual Register Map
 ; GCN-O1-NEXT:         Live Register Matrix
 ; GCN-O1-NEXT:         Bundle Machine CFG Edges
 ; GCN-O1-NEXT:         Spill Code Placement Analysis

 ; GCN-O1-OPTS-NEXT:         Rename Disconnected Subregister Components
 ; GCN-O1-OPTS-NEXT:         Machine Instruction Scheduler
 ; GCN-O1-OPTS-NEXT:         MachinePostDominator Tree Construction
 ; GCN-O1-OPTS-NEXT:         SI Whole Quad Mode
 ; GCN-O1-OPTS-NEXT:         Virtual Register Map
 ; GCN-O1-OPTS-NEXT:         Live Register Matrix
 ; GCN-O1-OPTS-NEXT:         SI Pre-allocate WWM Registers
 ; GCN-O1-OPTS-NEXT:         SI optimize exec mask operations pre-RA
-; GCN-O1-OPTS-NEXT:         SI Form memory clauses
 ; GCN-O1-OPTS-NEXT:         Machine Natural Loop Construction
 ; GCN-O1-OPTS-NEXT:         Machine Block Frequency Analysis
 ; GCN-O1-OPTS-NEXT:         Debug Variable Analysis
 ; GCN-O1-OPTS-NEXT:         Live Stack Slot Analysis
 ; GCN-O1-OPTS-NEXT:         Virtual Register Map
 ; GCN-O1-OPTS-NEXT:         Live Register Matrix
 ; GCN-O1-OPTS-NEXT:         Bundle Machine CFG Edges
 ; GCN-O1-OPTS-NEXT:         Spill Code Placement Analysis