[RISCV] Do not check PostRAScheduler in enablePostRAScheduler

[michaelmaitland]

On RISC-V, there are a few ways to control whether the PostMachineScheduler is enabled. If -enable-post-misched is passed or passed with a value of true, then the PostMachineScheduler is enabled. If it is passed with a value of false then the PostMachineScheduler is disabled. If the option is not passed at all, then RISCVSubtarget::enablePostRAMachineScheduler decides whether the pass should be enabled or not. TargetSubtargetInfo::enablePostRAScheduler and TargetSubtargetInfo::enablePostRAMachineScheduler who check the SchedModel value are not called by RISC-V backend.

RISCVSubtarget::enablePostRAMachineScheduler currently checks if the active scheduler model sets PostRAScheduler. If it is set to true by the scheduler model, then the pass is enabled. If it is not set to true by the scheduler model, then the value of UsePostRAScheduler subtarget feature is used.

I argue that the RISC-V backend should not use PostRAScheduler field of the scheduler model to control whether the PostMachineScheduler is enabled for the following reasons:

1. No other targets use this value to control whether PostMachineScheduler is enabled. They only use it to check whether the legacy PostRASchedulerList scheduler is enabled.

2. We can add the UsePostRAScheduler feature to the processor definition in RISCVProcessors.td to tie a processor to whether the pass should be enabled by default. This makes the feature and the sched model field redundant.

3. Since these options are redundant, we should prefer the feature, since we can set + and - on the feature, but the value of the scheduler cannot be controlled on the command line.

4. Keeping both options allows us to set the feature and the scheduler model value to conflicting values. Although the scheduler model value will win out, it feels awkward to allow it.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Michael Maitland (michaelmaitland)

Changes

On RISC-V, there are a few ways to control whether the PostMachineScheduler is enabled. If -enable-post-misched is passed or passed with a value of true, then the PostMachineScheduler is enabled. If it is passed with a value of false then the PostMachineScheduler is disabled. If the option is not passed at all, then TargetSubtargetInfo::enablePostRAMachineScheduler decides whether the pass should be enabled or not.

RISCVSubtarget::enablePostRAMachineScheduler currently checks if the active scheduler model sets PostRAScheduler. If it is set to true by the scheduler model, then the pass is enabled. If it is not set to true by the scheduler model, then the value of UsePostRAScheduler subtarget feature is used.

I argue that the RISC-V backend should not use PostRAScheduler field of the scheduler model to control whether the PostMachineScheduler is enabled for the following reasons:

1. No other targets use this value to control whether PostMachineScheduler is enabled. They only use it to check whether the legacy PostRASchedulerList scheduelr is enabled.

2. We can add the UsePostRAScheduler feature to the processor definition in RISCVProcessors.td to tie a processor to whether the pass should be enabled by default. This makes the feature and the sched model field redundant.

3. Since these options are redundant, we should prefer the feature, since we can set + and - on the feature, but the value of the scheduler cannot be controled on the command line.

4. Keeping both options allows us to set the feature and the scheduler model value to conflicting values. Although the scheduler model value will win out, it feels awkward to allow it.

There are no upstream subtargets that use the scheduler model PostRAScheduler field. If this patch lands, all downstream subtargets should replace PostRAScheduler with the UsePostRAScheduler feature in RISCVProcessors.td to acheive the desired functionality.

---

Full diff: https://github.com/llvm/llvm-project/pull/92781.diff

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVSubtarget.h (+1-3)

diff --git a/llvm/lib/Target/RISCV/RISCVSubtarget.h b/llvm/lib/Target/RISCV/RISCVSubtarget.h
index c880c9e921e0e..347c1bc3c278f 100644
--- a/llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ b/llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -121,9 +121,7 @@ class RISCVSubtarget : public RISCVGenSubtargetInfo {
   }
   bool enableMachineScheduler() const override { return true; }
 
-  bool enablePostRAScheduler() const override {
-    return getSchedModel().PostRAScheduler || UsePostRAScheduler;
-  }
+  bool enablePostRAScheduler() const override { return UsePostRAScheduler; }
 
   Align getPrefFunctionAlignment() const {
     return Align(TuneInfo->PrefFunctionAlignment);

[topperc]

Do we need to add FeaturePostRAScheduler to SiFive7, SiFiveP400, and SiFive600 in RISCVProcessors.td?

[topperc]

No other targets use this value to control whether PostMachineScheduler is enabled. They only use it to check whether the legacy PostRASchedulerList scheduelr is enabled.

No targets use these default implementations which would use the field from the scheduler for the PostRA MachineScheduler?

bool TargetSubtargetInfo::enablePostRAScheduler() const {                        
  return getSchedModel().PostRAScheduler;                                        
}                                                                                
                                                                                 
bool TargetSubtargetInfo::enablePostRAMachineScheduler() const {                 
  return enableMachineScheduler() && enablePostRAScheduler();                    
} 

[michaelmaitland]

No other targets use this value to control whether PostMachineScheduler is enabled. They only use it to check whether the legacy PostRASchedulerList scheduelr is enabled.

No targets use these default implementations which would use the field from the scheduler for the PostRA MachineScheduler?

bool TargetSubtargetInfo::enablePostRAScheduler() const {                        
  return getSchedModel().PostRAScheduler;                                        
}                                                                                
                                                                                 
bool TargetSubtargetInfo::enablePostRAMachineScheduler() const {                 
  return enableMachineScheduler() && enablePostRAScheduler();                    
} 

I didn't see this. I rephrase to targets that override enablePostRAScheduler or enablePostRAMachineScheduler do not use PostRAScheduler.

[wangpc-pp]

I think it's messy now as we have two ways to enable PostRAScheduler. For AArch64, which has overrided enablePostRAScheduler and only the feature value takes effect, but we can still see some models that set PostRAScheduler field in SchedMachineModel:

llvm-project/llvm/lib/Target/AArch64/AArch64SchedA510.td

Lines 17 to 29 in 6658e1a

	// Cortex-A510 machine model for scheduling and other instruction cost heuristics.
	def CortexA510Model : SchedMachineModel {
	let MicroOpBufferSize = 0; // The Cortex-A510 is an in-order processor
	let IssueWidth = 3; // It dual-issues under most circumstances
	let LoadLatency = 3; // Cycles for loads to access the cache.
	// Most loads have a latency of 2, but some have higher latencies.
	// 3 seems to be a good tradeoff
	let PostRAScheduler = 1; // Enable PostRA scheduler pass.
	let CompleteModel = 0; // Covers instructions applicable to Cortex-A510.
	// FIXME: Remove when all errors have been fixed.
	let FullInstRWOverlapCheck = 0;
	}

I think it worths unifying the way for RISCV, and I do prefer the feature way (I think you have spoken my mind :-)).
We should add a release note, add some comments and so on to emphasize that the setting of SchedMachineModel's PostRAScheduler won't do any difference and we should add UsePostRAScheduler to Processor definitions if we want to enable it.

[topperc]

Don't SiFiveP400, SiFiveP600, and SiFive7 all have let PostRAScheduler = 1;? Why don't we need to change RISCVProcessors.td for them?

[topperc]

This patch is failing 2 tests

LLVM :: CodeGen/RISCV/machine-combiner.ll

LLVM :: CodeGen/RISCV/short-forward-branch-opt.ll

[michaelmaitland]

Don't SiFiveP400, SiFiveP600, and SiFive7 all have let PostRAScheduler = 1;? Why don't we need to change RISCVProcessors.td for them?

I am dumb and looked for PostRAScheduler = 1 instead of PostRAScheduler = true. Will update patch.

[topperc]

LGTM

[wangpc-pp]

LGTM.