[RISCV] Introduce pass to promote double constants to a global array #160536
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As discussed in llvm#153402, we have inefficiences in handling constant pool access that are difficult to address. Using an IR pass to promote double constants to a global allows a higher degree of control of code generation for these accesses, resulting in improved performance on benchmarks that might otherwise have high register pressure due to accessing constant pool values separately rather than via a common base. Directly promoting double constants to separate global values and relying on the global merger to do a sensible thing would be one potential avenue to explore, but it is _not_ done in this version of the patch because: * The global merger pass needs fixes. For instance it claims to be a function pass, yet all of the work is done in initialisation. This means that attempts by backends to schedule it after a given module pass don't actually work as expected. * The heuristics used can impact codegen unexpectedly, so I worry that tweaking it to get the behaviour desired for promoted constants may lead to other issues. This may be completely tractable though. Now that llvm#159352 has landed, the impact on terms if dynamically executed instructions is slightly smaller (as we are starting from a better baseline), but still worthwhile in lbm and nab from SPEC. Results below are for rva22u64: ``` Benchmark Baseline This PR Diff (%) ============================================================ 500.perlbench_r 180667466583 180667466661 0.00% 502.gcc_r 221281439537 221277561043 -0.00% 505.mcf_r 134656203905 134656204017 0.00% 508.namd_r 217646645213 217616374477 -0.01% 510.parest_r 291730242760 291917069933 0.06% 511.povray_r 30982459833 31101871667 0.39% 519.lbm_r 91217999812 89029313608 -2.40% 520.omnetpp_r 137705551722 138044390554 0.25% 523.xalancbmk_r 284733326286 284728940808 -0.00% 525.x264_r 379107521545 379100249676 -0.00% 526.blender_r 659391437704 659446918261 0.01% 531.deepsjeng_r 350038121655 350038121654 -0.00% 538.imagick_r 238568679271 238560769465 -0.00% 541.leela_r 405654701351 405660852862 0.00% 544.nab_r 398215801713 391380811065 -1.72% 557.xz_r 129832192046 129832192047 0.00% ```
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-llvm-globalisel Author: Alex Bradbury (asb) ChangesAs discussed in #153402, we have inefficiences in handling constant pool access that are difficult to address. Using an IR pass to promote double constants to a global allows a higher degree of control of code generation for these accesses, resulting in improved performance on benchmarks that might otherwise have high register pressure due to accessing constant pool values separately rather than via a common base. Directly promoting double constants to separate global values and relying on the global merger to do a sensible thing would be one potential avenue to explore, but it is not done in this version of the patch because:
Now that #159352 has landed, the impact on terms if dynamically executed instructions is slightly smaller (as we are starting from a better baseline), but still worthwhile in lbm and nab from SPEC. Results below are for rva22u64: Notes for reviewers:
Patch is 71.50 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/160536.diff 11 Files Affected:
diff --git a/llvm/lib/Target/RISCV/CMakeLists.txt b/llvm/lib/Target/RISCV/CMakeLists.txt
index 0ff178e1f1959..8702b9e63f867 100644
--- a/llvm/lib/Target/RISCV/CMakeLists.txt
+++ b/llvm/lib/Target/RISCV/CMakeLists.txt
@@ -59,6 +59,7 @@ add_llvm_target(RISCVCodeGen
RISCVOptWInstrs.cpp
RISCVPostRAExpandPseudoInsts.cpp
RISCVPushPopOptimizer.cpp
+ RISCVPromoteConstant.cpp
RISCVRedundantCopyElimination.cpp
RISCVRegisterInfo.cpp
RISCVSelectionDAGInfo.cpp
diff --git a/llvm/lib/Target/RISCV/RISCV.h b/llvm/lib/Target/RISCV/RISCV.h
index ae9410193efe1..5b0ce521409ad 100644
--- a/llvm/lib/Target/RISCV/RISCV.h
+++ b/llvm/lib/Target/RISCV/RISCV.h
@@ -19,6 +19,7 @@
namespace llvm {
class FunctionPass;
+class ModulePass;
class InstructionSelector;
class PassRegistry;
class RISCVRegisterBankInfo;
@@ -111,6 +112,8 @@ void initializeRISCVO0PreLegalizerCombinerPass(PassRegistry &);
FunctionPass *createRISCVPreLegalizerCombiner();
void initializeRISCVPreLegalizerCombinerPass(PassRegistry &);
+ModulePass *createRISCVPromoteConstantPass();
+
FunctionPass *createRISCVVLOptimizerPass();
void initializeRISCVVLOptimizerPass(PassRegistry &);
diff --git a/llvm/lib/Target/RISCV/RISCVPromoteConstant.cpp b/llvm/lib/Target/RISCV/RISCVPromoteConstant.cpp
new file mode 100644
index 0000000000000..44a51eddc8057
--- /dev/null
+++ b/llvm/lib/Target/RISCV/RISCVPromoteConstant.cpp
@@ -0,0 +1,212 @@
+//==- RISCVPromoteConstant.cpp - Promote constant fp to global for RISC-V --==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "RISCV.h"
+#include "RISCVSubtarget.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Statistic.h"
+#include "llvm/CodeGen/TargetLowering.h"
+#include "llvm/CodeGen/TargetPassConfig.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/Constant.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/GlobalValue.h"
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/InstIterator.h"
+#include "llvm/IR/Instruction.h"
+#include "llvm/IR/Instructions.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/Type.h"
+#include "llvm/InitializePasses.h"
+#include "llvm/Pass.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Debug.h"
+
+using namespace llvm;
+
+#define DEBUG_TYPE "riscv-promote-const"
+
+STATISTIC(NumPromoted, "Number of promoted constants");
+STATISTIC(NumPromotedUses, "Number of promoted constants uses");
+
+namespace {
+
+class RISCVPromoteConstant : public ModulePass {
+public:
+ static char ID;
+ RISCVPromoteConstant() : ModulePass(ID) {}
+
+ StringRef getPassName() const override { return "RISC-V Promote Constant"; }
+
+ void getAnalysisUsage(AnalysisUsage &AU) const override {
+ AU.addRequired<TargetPassConfig>();
+ AU.setPreservesCFG();
+ }
+
+ /// Iterate over the functions and promote the double fp constants that
+ /// would otherwise go into the constant pool to a constant array.
+ bool runOnModule(Module &M) override {
+ LLVM_DEBUG(dbgs() << getPassName() << '\n');
+ // TargetMachine and Subtarget are needed to query isFPImmlegal. Get them
+ // from TargetPassConfig.
+ const TargetPassConfig &TPC = getAnalysis<TargetPassConfig>();
+ const TargetMachine &TM = TPC.getTM<TargetMachine>();
+ if (skipModule(M))
+ return false;
+ bool Changed = false;
+ for (auto &MF : M) {
+ const RISCVSubtarget &ST = TM.getSubtarget<RISCVSubtarget>(MF);
+ const RISCVTargetLowering *TLI = ST.getTargetLowering();
+ Changed |= runOnFunction(MF, TLI);
+ }
+ return Changed;
+ }
+
+private:
+ bool runOnFunction(Function &F, const RISCVTargetLowering *TLI);
+};
+} // end anonymous namespace
+
+char RISCVPromoteConstant::ID = 0;
+
+ModulePass *llvm::createRISCVPromoteConstantPass() {
+ return new RISCVPromoteConstant();
+}
+
+bool RISCVPromoteConstant::runOnFunction(Function &F,
+ const RISCVTargetLowering *TLI) {
+ // Bail out and make no transformation if the target doesn't support
+ // doubles, or if we're not targeting RV64 as we currently see some
+ // regressions for those targets.
+ if (!TLI->isTypeLegal(MVT::f64) || !TLI->isTypeLegal(MVT::i64))
+ return false;
+
+ // Collect all unique double constants used in the function, and track their
+ // offset within the newly created global array. Also track uses that will
+ // be replaced later.
+ DenseMap<ConstantFP *, unsigned> ConstantMap;
+ SmallVector<Constant *, 16> ConstantVector;
+ DenseMap<ConstantFP *, SmallVector<Use *, 8>> UsesInFunc;
+
+ for (Instruction &I : instructions(F)) {
+ // PHI nodes are handled specially in a second loop below.
+ if (isa<PHINode>(I))
+ continue;
+ for (Use &U : I.operands()) {
+ if (auto *C = dyn_cast<ConstantFP>(U.get())) {
+ if (C->getType()->isDoubleTy()) {
+ if (TLI->isFPImmLegal(C->getValueAPF(), MVT::f64,
+ /*ForCodeSize*/ false))
+ continue;
+ UsesInFunc[C].push_back(&U);
+ if (ConstantMap.find(C) == ConstantMap.end()) {
+ ConstantMap[C] = ConstantVector.size();
+ ConstantVector.push_back(C);
+ ++NumPromoted;
+ }
+ }
+ }
+ }
+ }
+
+ // Collect uses from PHI nodes after other uses, because when transforming
+ // the function, we handle PHI uses afterwards.
+ for (BasicBlock &BB : F) {
+ for (PHINode &PN : BB.phis()) {
+ for (unsigned i = 0, e = PN.getNumIncomingValues(); i != e; ++i) {
+ if (auto *C = dyn_cast<ConstantFP>(PN.getIncomingValue(i))) {
+ if (C->getType()->isDoubleTy()) {
+ if (TLI->isFPImmLegal(C->getValueAPF(), MVT::f64,
+ /*ForCodeSize*/ false))
+ continue;
+ UsesInFunc[C].push_back(&PN.getOperandUse(i));
+ if (ConstantMap.find(C) == ConstantMap.end()) {
+ ConstantMap[C] = ConstantVector.size();
+ ConstantVector.push_back(C);
+ ++NumPromoted;
+ }
+ }
+ }
+ }
+ }
+ }
+
+ // Bail out if no promotable constants found.
+ if (ConstantVector.empty())
+ return false;
+
+ // Create a global array containing the promoted constants.
+ Module *M = F.getParent();
+ Type *DoubleTy = Type::getDoubleTy(M->getContext());
+ ArrayType *ArrayTy = ArrayType::get(DoubleTy, ConstantVector.size());
+ Constant *GlobalArrayInitializer =
+ ConstantArray::get(ArrayTy, ConstantVector);
+
+ auto *GlobalArray = new GlobalVariable(
+ *M, ArrayTy,
+ /*isConstant=*/true, GlobalValue::InternalLinkage, GlobalArrayInitializer,
+ ".promoted_doubles." + F.getName());
+
+ // Create GEP for the base pointer in the function entry.
+ IRBuilder<> EntryBuilder(&F.getEntryBlock().front());
+ Value *BasePtr = EntryBuilder.CreateConstInBoundsGEP2_64(
+ GlobalArray->getValueType(), GlobalArray, 0, 0, "doubles.base");
+
+ // A cache to hold the loaded value for a given constant within a basic block.
+ DenseMap<std::pair<ConstantFP *, BasicBlock *>, Value *> LocalLoads;
+
+ // Replace all uses with the loaded value.
+ for (Constant *ConstVal : ConstantVector) {
+ auto *Const = cast<ConstantFP>(ConstVal);
+ const auto &Uses = UsesInFunc.at(Const);
+ unsigned Idx = ConstantMap.at(Const);
+
+ for (Use *U : Uses) {
+ Instruction *UserInst = cast<Instruction>(U->getUser());
+ BasicBlock *InsertionBB;
+ Instruction *InsertionPt;
+
+ if (auto *PN = dyn_cast<PHINode>(UserInst)) {
+ // If the user is a PHI node, we must insert the load in the
+ // corresponding predecessor basic block, before its terminator.
+ unsigned OperandIdx = U->getOperandNo();
+ InsertionBB = PN->getIncomingBlock(OperandIdx);
+ InsertionPt = InsertionBB->getTerminator();
+ } else {
+ // For any other instruction, we can insert the load right before it.
+ InsertionBB = UserInst->getParent();
+ InsertionPt = UserInst;
+ }
+
+ auto CacheKey = std::make_pair(Const, InsertionBB);
+ Value *LoadedVal = nullptr;
+
+ // Re-use a load if it exists in the insertion block.
+ if (LocalLoads.count(CacheKey)) {
+ LoadedVal = LocalLoads.at(CacheKey);
+ } else {
+ // Otherwise, create a new GEP and Load at the correct insertion point.
+ IRBuilder<> Builder(InsertionPt);
+ Value *ElementPtr = Builder.CreateConstInBoundsGEP1_64(
+ DoubleTy, BasePtr, Idx, "double.addr");
+ LoadedVal = Builder.CreateLoad(DoubleTy, ElementPtr, "double.val");
+
+ // Cache the newly created load for this block.
+ LocalLoads[CacheKey] = LoadedVal;
+ }
+
+ U->set(LoadedVal);
+ ++NumPromotedUses;
+ }
+ }
+
+ return true;
+}
diff --git a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
index f81b1e1260ee3..1c0f4d3ad041b 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -463,6 +463,8 @@ void RISCVPassConfig::addIRPasses() {
}
bool RISCVPassConfig::addPreISel() {
+ if (TM->getOptLevel() != CodeGenOptLevel::None)
+ addPass(createRISCVPromoteConstantPass());
if (TM->getOptLevel() != CodeGenOptLevel::None) {
// Add a barrier before instruction selection so that we will not get
// deleted block address after enabling default outlining. See D99707 for
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vararg.ll b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vararg.ll
index 74961d12c1c85..7233ce6d593d0 100644
--- a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vararg.ll
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vararg.ll
@@ -67,8 +67,8 @@ define i32 @va1(ptr %fmt, ...) {
; RV32-NEXT: G_VASTART [[FRAME_INDEX1]](p0) :: (store (s32) into %ir.va)
; RV32-NEXT: [[LOAD:%[0-9]+]]:_(p0) = G_LOAD [[FRAME_INDEX1]](p0) :: (dereferenceable load (p0) from %ir.va)
; RV32-NEXT: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 4
- ; RV32-NEXT: %20:_(p0) = nuw nusw inbounds G_PTR_ADD [[LOAD]], [[C1]](s32)
- ; RV32-NEXT: G_STORE %20(p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va)
+ ; RV32-NEXT: [[PTR_ADD7:%[0-9]+]]:_(p0) = nuw nusw inbounds G_PTR_ADD [[LOAD]], [[C1]](s32)
+ ; RV32-NEXT: G_STORE [[PTR_ADD7]](p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va)
; RV32-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[LOAD]](p0) :: (load (s32) from %ir.argp.cur)
; RV32-NEXT: $x10 = COPY [[LOAD1]](s32)
; RV32-NEXT: PseudoRET implicit $x10
@@ -105,8 +105,8 @@ define i32 @va1(ptr %fmt, ...) {
; RV64-NEXT: G_VASTART [[FRAME_INDEX1]](p0) :: (store (s64) into %ir.va)
; RV64-NEXT: [[LOAD:%[0-9]+]]:_(p0) = G_LOAD [[FRAME_INDEX1]](p0) :: (dereferenceable load (p0) from %ir.va, align 4)
; RV64-NEXT: [[C1:%[0-9]+]]:_(s64) = G_CONSTANT i64 4
- ; RV64-NEXT: %20:_(p0) = nuw nusw inbounds G_PTR_ADD [[LOAD]], [[C1]](s64)
- ; RV64-NEXT: G_STORE %20(p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va, align 4)
+ ; RV64-NEXT: [[PTR_ADD7:%[0-9]+]]:_(p0) = nuw nusw inbounds G_PTR_ADD [[LOAD]], [[C1]](s64)
+ ; RV64-NEXT: G_STORE [[PTR_ADD7]](p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va, align 4)
; RV64-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[LOAD]](p0) :: (load (s32) from %ir.argp.cur)
; RV64-NEXT: [[ANYEXT:%[0-9]+]]:_(s64) = G_ANYEXT [[LOAD1]](s32)
; RV64-NEXT: $x10 = COPY [[ANYEXT]](s64)
@@ -687,8 +687,8 @@ define i64 @va2(ptr %fmt, ...) nounwind {
; RV32-NEXT: [[AND:%[0-9]+]]:_(s32) = G_AND [[ADD]], [[C2]]
; RV32-NEXT: [[INTTOPTR:%[0-9]+]]:_(p0) = G_INTTOPTR [[ADD]](s32)
; RV32-NEXT: [[C3:%[0-9]+]]:_(s32) = G_CONSTANT i32 8
- ; RV32-NEXT: %25:_(p0) = nuw nusw inbounds G_PTR_ADD [[INTTOPTR]], [[C3]](s32)
- ; RV32-NEXT: G_STORE %25(p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va)
+ ; RV32-NEXT: [[PTR_ADD7:%[0-9]+]]:_(p0) = nuw nusw inbounds G_PTR_ADD [[INTTOPTR]], [[C3]](s32)
+ ; RV32-NEXT: G_STORE [[PTR_ADD7]](p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va)
; RV32-NEXT: [[INTTOPTR1:%[0-9]+]]:_(p0) = G_INTTOPTR [[AND]](s32)
; RV32-NEXT: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[INTTOPTR1]](p0) :: (load (s64) from %ir.3)
; RV32-NEXT: [[UV:%[0-9]+]]:_(s32), [[UV1:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[LOAD1]](s64)
@@ -733,8 +733,8 @@ define i64 @va2(ptr %fmt, ...) nounwind {
; RV64-NEXT: [[AND:%[0-9]+]]:_(s32) = G_AND [[ADD]], [[C2]]
; RV64-NEXT: [[INTTOPTR:%[0-9]+]]:_(p0) = G_INTTOPTR [[ADD]](s32)
; RV64-NEXT: [[C3:%[0-9]+]]:_(s64) = G_CONSTANT i64 8
- ; RV64-NEXT: %25:_(p0) = nuw nusw inbounds G_PTR_ADD [[INTTOPTR]], [[C3]](s64)
- ; RV64-NEXT: G_STORE %25(p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va, align 4)
+ ; RV64-NEXT: [[PTR_ADD7:%[0-9]+]]:_(p0) = nuw nusw inbounds G_PTR_ADD [[INTTOPTR]], [[C3]](s64)
+ ; RV64-NEXT: G_STORE [[PTR_ADD7]](p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va, align 4)
; RV64-NEXT: [[INTTOPTR1:%[0-9]+]]:_(p0) = G_INTTOPTR [[AND]](s32)
; RV64-NEXT: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[INTTOPTR1]](p0) :: (load (s64) from %ir.3)
; RV64-NEXT: $x10 = COPY [[LOAD1]](s64)
@@ -974,8 +974,8 @@ define i64 @va3(i32 %a, i64 %b, ...) nounwind {
; RV32-NEXT: [[AND:%[0-9]+]]:_(s32) = G_AND [[ADD]], [[C2]]
; RV32-NEXT: [[INTTOPTR:%[0-9]+]]:_(p0) = G_INTTOPTR [[ADD]](s32)
; RV32-NEXT: [[C3:%[0-9]+]]:_(s32) = G_CONSTANT i32 8
- ; RV32-NEXT: %24:_(p0) = nuw nusw inbounds G_PTR_ADD [[INTTOPTR]], [[C3]](s32)
- ; RV32-NEXT: G_STORE %24(p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va)
+ ; RV32-NEXT: [[PTR_ADD5:%[0-9]+]]:_(p0) = nuw nusw inbounds G_PTR_ADD [[INTTOPTR]], [[C3]](s32)
+ ; RV32-NEXT: G_STORE [[PTR_ADD5]](p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va)
; RV32-NEXT: [[INTTOPTR1:%[0-9]+]]:_(p0) = G_INTTOPTR [[AND]](s32)
; RV32-NEXT: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[INTTOPTR1]](p0) :: (load (s64) from %ir.3)
; RV32-NEXT: [[ADD1:%[0-9]+]]:_(s64) = G_ADD [[MV]], [[LOAD1]]
@@ -1020,8 +1020,8 @@ define i64 @va3(i32 %a, i64 %b, ...) nounwind {
; RV64-NEXT: [[AND:%[0-9]+]]:_(s32) = G_AND [[ADD]], [[C2]]
; RV64-NEXT: [[INTTOPTR:%[0-9]+]]:_(p0) = G_INTTOPTR [[ADD]](s32)
; RV64-NEXT: [[C3:%[0-9]+]]:_(s64) = G_CONSTANT i64 8
- ; RV64-NEXT: %25:_(p0) = nuw nusw inbounds G_PTR_ADD [[INTTOPTR]], [[C3]](s64)
- ; RV64-NEXT: G_STORE %25(p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va, align 4)
+ ; RV64-NEXT: [[PTR_ADD6:%[0-9]+]]:_(p0) = nuw nusw inbounds G_PTR_ADD [[INTTOPTR]], [[C3]](s64)
+ ; RV64-NEXT: G_STORE [[PTR_ADD6]](p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va, align 4)
; RV64-NEXT: [[INTTOPTR1:%[0-9]+]]:_(p0) = G_INTTOPTR [[AND]](s32)
; RV64-NEXT: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[INTTOPTR1]](p0) :: (load (s64) from %ir.3)
; RV64-NEXT: [[ADD1:%[0-9]+]]:_(s64) = G_ADD [[COPY1]], [[LOAD1]]
@@ -1724,8 +1724,8 @@ define i32 @va_large_stack(ptr %fmt, ...) {
; RV32-NEXT: G_VASTART [[FRAME_INDEX2]](p0) :: (store (s32) into %ir.va)
; RV32-NEXT: [[LOAD:%[0-9]+]]:_(p0) = G_LOAD [[FRAME_INDEX2]](p0) :: (dereferenceable load (p0) from %ir.va)
; RV32-NEXT: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 4
- ; RV32-NEXT: %21:_(p0) = nuw nusw inbounds G_PTR_ADD [[LOAD]], [[C1]](s32)
- ; RV32-NEXT: G_STORE %21(p0), [[FRAME_INDEX2]](p0) :: (store (p0) into %ir.va)
+ ; RV32-NEXT: [[PTR_ADD7:%[0-9]+]]:_(p0) = nuw nusw inbounds G_PTR_ADD [[LOAD]], [[C1]](s32)
+ ; RV32-NEXT: G_STORE [[PTR_ADD7]](p0), [[FRAME_INDEX2]](p0) :: (store (p0) into %ir.va)
; RV32-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[LOAD]](p0) :: (load (s32) from %ir.argp.cur)
; RV32-NEXT: $x10 = COPY [[LOAD1]](s32)
; RV32-NEXT: PseudoRET implicit $x10
@@ -1763,8 +1763,8 @@ define i32 @va_large_stack(ptr %fmt, ...) {
; RV64-NEXT: G_VASTART [[FRAME_INDEX2]](p0) :: (store (s64) into %ir.va)
; RV64-NEXT: [[LOAD:%[0-9]+]]:_(p0) = G_LOAD [[FRAME_INDEX2]](p0) :: (dereferenceable load (p0) from %ir.va, align 4)
; RV64-NEXT: [[C1:%[0-9]+]]:_(s64) = G_CONSTANT i64 4
- ; RV64-NEXT: %21:_(p0) = nuw nusw inbounds G_PTR_ADD [[LOAD]], [[C1]](s64)
- ; RV64-NEXT: G_STORE %21(p0), [[FRAME_INDEX2]](p0) :: (store (p0) into %ir.va, align 4)
+ ; RV64-NEXT: [[PTR_ADD7:%[0-9]+]]:_(p0) = nuw nusw inbounds G_PTR_ADD [[LOAD]], [[C1]](s64)
+ ; RV64-NEXT: G_STORE [[PTR_ADD7]](p0), [[FRAME_INDEX2]](p0) :: (store (p0) into %ir.va, align 4)
; RV64-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[LOAD]](p0) :: (load (s32) from %ir.argp.cur)
; RV64-NEXT: [[ANYEXT:%[0-9]+]]:_(s64) = G_ANYEXT [[LOAD1]](s32)
; RV64-NEXT: $x10 = COPY [[ANYEXT]](s64)
diff --git a/llvm/test/CodeGen/RISCV/O3-pipeline.ll b/llvm/test/CodeGen/RISCV/O3-pipeline.ll
index ea08061221fd4..a982de8301e2c 100644
--- a/llvm/test/CodeGen/RISCV/O3-pipeline.ll
+++ b/llvm/test/CodeGen/RISCV/O3-pipeline.ll
@@ -75,6 +75,7 @@
; CHECK-NEXT: CodeGen Prepare
; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Exception handling preparation
+; CHECK-NEXT: RISC-V Promote Constant
; CHECK-NEXT: A No-Op Barrier Pass
; CHECK-NEXT: FunctionPass Manager
; CHECK-NEXT: Merge internal globals
diff --git a/llvm/test/CodeGen/RISCV/double-imm.ll b/llvm/test/CodeGen/RISCV/double-imm.ll
index 6f7c30edba3ea..3d1b0d8cc9658 100644
--- a/llvm/test/CodeGen/RISCV/double-imm.ll
+++ b/llvm/test/CodeGen/RISCV/double-imm.ll
@@ -17,8 +17,8 @@ define double @double_imm() nounwind {
;
; CHECK64D-LABEL: double_imm:
; CHECK64D: # %bb.0:
-; CHECK64D-NEXT: lui a0, %hi(.LCPI0_0)
-; CHECK64D-NEXT: fld fa0, %lo(.LCPI0_0)(a0)
+; CHECK64D-NEXT: lui a0, %hi(.promoted_doubles.double_imm)
+; CHECK64D-NEXT: fld fa0, %lo(.promoted_doubles.double_imm)(a0)
; CHECK64D-NEXT: ret
;
; CHECKRV32ZDINX-LABEL: double_imm:
@@ -31,8 +31,8 @@ define double @double_imm() nounwind {
;
; CHECKRV64ZDINX-LABEL: double_imm:
; CHECKRV64ZDINX: # %bb.0:
-; CHECKRV64ZDINX-NEXT: lui a0, %hi(.LCPI0_0)
-; CHECKRV64ZDINX-NEXT: ld a0, %lo(.LCPI0_0)(a0)
+; CHECKRV64ZDINX-NEXT: lui a0, %hi(.promoted_doubles.double_imm)
+; CHECKRV64ZDINX-NEXT: ld a0, %lo(.promoted_doubles.double_imm)(a0)
; CHECKRV64ZDINX-NEXT: ret
ret double 3.1415926535897931159979634685441851615905761718750
}
diff --git a/llvm/test/CodeGen/RISCV/double-select-fcmp.ll b/llvm/test/CodeGen/RISCV/double-select-fcmp.ll
index 1deea55b083ce..0d4a69b69fb8c 100644
--- a/llvm/test/CodeGen/RISCV/double-select-fcmp.ll
+++ b/llvm/test/CodeGen/RISCV/double-select-fcmp.ll
@@ -1,8 +1,8 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mtriple=riscv32 -mattr=+d -verify-machineinstrs < %s \
-; RUN: -target-abi=ilp32d | FileCheck %s
+; RUN: -target-abi=ilp32d | FileCheck -check-prefixes=CHECK,CHECKRV32D %s
; RUN: llc -mtriple=riscv64 -mattr=+d -verify-machineinstrs < %s \
-; RUN: -target-abi=lp64d | FileCheck %s
+; RUN: -target-abi=lp64d | FileCheck -check-prefixes=CHECK,CHECKRV64D %s
; RUN: llc -mtriple=riscv32 -mattr=+zdinx -verify-machineinstrs < %s \
; RUN: -target-abi=ilp32 | FileCheck --check-prefix=CHECKRV32ZDINX %s
; RUN: llc -mtriple=riscv64 -mattr=+zdinx -verify-machineinstrs < %s \
@@ -640,6 +640,39 @@ define signext i32 @select_fcmp_uge_1_2(double %a, double %b) nounwind {
}
define double @CascadedSelect(double noundef %a) {
+; CHECKRV32D-LABEL: CascadedSelect:
+; CHECKRV32D: # %bb.0: # %entry
+; CHECKRV32D-NEXT: lui a0, %hi(.LCPI20_0)
+; CHECKRV32D-NEXT: fld fa5, %lo(.LCPI20_0)(a0)
+; CHECKRV32D-NEXT: flt.d a0, fa5, fa0
+; CHECKRV32D-NEXT: bnez a0, .LBB20_3
+; CHECKRV32D-NEXT: # %bb.1: # %entry
+; CHECKRV32D-NEXT: fcvt.d.w fa5, zero
+; CHECKRV32D-NEXT: flt.d a0, fa0, fa5
+; CHECKRV32D-NEXT: bnez a0, .LBB20_3
+; CHECKRV32D-NEXT: # %bb.2: # %entry
+; CHECKRV32D-NEXT: fmv.d fa5, fa0
+; CHECKRV32D-NEXT: .LBB20_3: # %entry
+; CHECKRV32D-NEXT: fmv.d fa0, fa5
+; CHECKRV32D-NEXT: ret
+;
+; CHECKRV64D-LABEL: CascadedSelect:
+; CHECK...
[truncated]
|
lukel97
reviewed
Sep 24, 2025
preames
reviewed
Sep 24, 2025
| } | ||
|
|
||
| // Collect uses from PHI nodes after other uses, because when transforming | ||
| // the function, we handle PHI uses afterwards. |
There was a problem hiding this comment.
This comment isn't really making sense to me? Why does it matter than the other lowering does two phases? Is there something about the order of the resulting global which is critical?
There was a problem hiding this comment.
Yes it's the awkward issue related to choosing a different insertion point when the use is a PHI or for a "normal" use.
But taking a step back, I feel I've failed to simplify this as much as possible. I'd started like AArch64ConstantPromotion with doing minimal inserts based on the dominator tree but decided it wasn't worth it initially, but never revisisted if there's any benefit to inserting as close to the use as possible. Things are kept much simpler by always inserting in the first possible insertion point (impact described in forthcoming top-level PR comment).
| IRBuilder<> Builder(InsertionPt); | ||
| Value *ElementPtr = Builder.CreateConstInBoundsGEP1_64( | ||
| DoubleTy, BasePtr, Idx, "double.addr"); | ||
| LoadedVal = Builder.CreateLoad(DoubleTy, ElementPtr, "double.val"); |
There was a problem hiding this comment.
The loads will be created with appropraite alignment - but of course the lack of an IR->IR test means that wasn't obvious! I've added tests now, which confirms this.
| ; RV32-NEXT: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 4 | ||
| ; RV32-NEXT: %20:_(p0) = nuw nusw inbounds G_PTR_ADD [[LOAD]], [[C1]](s32) | ||
| ; RV32-NEXT: G_STORE %20(p0), [[FRAME_INDEX1]](p0) :: (store (p0) into %ir.va) | ||
| ; RV32-NEXT: [[PTR_ADD7:%[0-9]+]]:_(p0) = nuw nusw inbounds G_PTR_ADD [[LOAD]], [[C1]](s32) |
There was a problem hiding this comment.
Is this change actually from the review, or just a stale auto-update? It sorta looks like the later.
| ; CHECK64D: # %bb.0: | ||
| ; CHECK64D-NEXT: lui a0, %hi(.LCPI0_0) | ||
| ; CHECK64D-NEXT: fld fa0, %lo(.LCPI0_0)(a0) | ||
| ; CHECK64D-NEXT: lui a0, %hi(.promoted_doubles.double_imm) |
There was a problem hiding this comment.
Hm, looks like maybe we shouldn't apply the transform unless there's more than one double in the function?
arsenm
reviewed
Sep 25, 2025
| if (isa<PHINode>(I)) | ||
| continue; | ||
| for (Use &U : I.operands()) { | ||
| if (auto *C = dyn_cast<ConstantFP>(U.get())) { |
There was a problem hiding this comment.
What about vector constants?
There was a problem hiding this comment.
Just to confirm: Is this a concern about correctness or a suggestion for another profitable transform? Fully agreed it could be worthwhile doing promotion for vector constants and I'd like to return to that, but I'm trying to keep this very narrowly scoped initially.
|
|
||
| namespace { | ||
|
|
||
| class RISCVPromoteConstant : public ModulePass { |
There was a problem hiding this comment.
Also add new pass manager version
There was a problem hiding this comment.
Based on my understanding form WritingAnLLVMNewPMPass.rst, I'd be writing a version deriving from PassInfoMixIn? This is more future proof, but I'm also not seeing any of the other RISC-V specific passes having done this yet.
There was a problem hiding this comment.
On this particular suggestion, I would actively prefer this be done in a follow on change. We do not have new pass manager versions of most RISCV codegen passes, and we should do these together.
|
|
||
| bool RISCVPromoteConstant::runOnFunction(Function &F, | ||
| const RISCVTargetLowering *TLI) { | ||
| // Bail out and make no transformation if the target doesn't support |
There was a problem hiding this comment.
Need to skip optnone functions / skipFunction
There was a problem hiding this comment.
skipFunction isn't available in a ModulePass, but I've added a skip if OptNone (and you can see if call check skipModule in runOnModule).
mshockwave
reviewed
Sep 25, 2025
|
I've just pushed changes that address almost all review comments on the code directly. I will merge in main and recheck for noise from test regeneration. Summary of changes:
I found that moving to insert the constant load at the earliest insertion point in the BB actually improved some benchmarks vs before. I believe this is by chance more than anything else (showing there's definitely scope for more improvements here...) and eyeballing changes I see static codegen differences where you end up with something slightly worse. But starting with the simpler logic and implementation makes sense to me, especially as there's no negative impact on performance. The prior version of this patch: The version with the simplified logic (vs the same baseline): |
topperc
reviewed
Oct 1, 2025
topperc
reviewed
Oct 1, 2025
preames
reviewed
Oct 7, 2025
|
|
||
| namespace { | ||
|
|
||
| class RISCVPromoteConstant : public ModulePass { |
There was a problem hiding this comment.
On this particular suggestion, I would actively prefer this be done in a follow on change. We do not have new pass manager versions of most RISCV codegen passes, and we should do these together.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #153402, we have inefficiences in handling constant pool access that are difficult to address. Using an IR pass to promote double constants to a global allows a higher degree of control of code generation for these accesses, resulting in improved performance on benchmarks that might otherwise have high register pressure due to accessing constant pool values separately rather than via a common base.
Directly promoting double constants to separate global values and relying on the global merger to do a sensible thing would be one potential avenue to explore, but it is not done in this version of the patch because:
Now that #159352 has landed, the impact on terms if dynamically executed instructions is slightly smaller (as we are starting from a better baseline), but still worthwhile in lbm and nab from SPEC. Results below are for rva22u64:
Notes for reviewers: