[NFC] [SPIRV] Add SPIRVCombinerHelper and refactor pre legalizer combiner to use it #162735
+104
−76
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
|
@llvm/pr-subscribers-backend-spir-v Author: Kaitlin Peng (kmpeng) ChangesLots of the code/structure was based off Tasks completed:
Full diff: https://github.com/llvm/llvm-project/pull/162735.diff 5 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/CMakeLists.txt b/llvm/lib/Target/SPIRV/CMakeLists.txt
index 46afe03648531..eab7b213756b3 100644
--- a/llvm/lib/Target/SPIRV/CMakeLists.txt
+++ b/llvm/lib/Target/SPIRV/CMakeLists.txt
@@ -36,6 +36,7 @@ add_llvm_target(SPIRVCodeGen
SPIRVMetadata.cpp
SPIRVModuleAnalysis.cpp
SPIRVStructurizer.cpp
+ SPIRVCombinerHelper.cpp
SPIRVPreLegalizer.cpp
SPIRVPreLegalizerCombiner.cpp
SPIRVPostLegalizer.cpp
diff --git a/llvm/lib/Target/SPIRV/SPIRVCombine.td b/llvm/lib/Target/SPIRV/SPIRVCombine.td
index 6f726e024de52..fde56c4d3632a 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCombine.td
+++ b/llvm/lib/Target/SPIRV/SPIRVCombine.td
@@ -11,8 +11,8 @@ include "llvm/Target/GlobalISel/Combine.td"
def vector_length_sub_to_distance_lowering : GICombineRule <
(defs root:$root),
(match (wip_match_opcode G_INTRINSIC):$root,
- [{ return matchLengthToDistance(*${root}, MRI); }]),
- (apply [{ applySPIRVDistance(*${root}, MRI, B); }])
+ [{ return Helper.matchLengthToDistance(*${root}); }]),
+ (apply [{ Helper.applySPIRVDistance(*${root}); }])
>;
def SPIRVPreLegalizerCombiner
diff --git a/llvm/lib/Target/SPIRV/SPIRVCombinerHelper.cpp b/llvm/lib/Target/SPIRV/SPIRVCombinerHelper.cpp
new file mode 100644
index 0000000000000..07f6bac9f1f51
--- /dev/null
+++ b/llvm/lib/Target/SPIRV/SPIRVCombinerHelper.cpp
@@ -0,0 +1,60 @@
+
+//===-- SPIRVCombinerHelper.cpp ---------------------------------*- C++ -*-===//
+//
+// 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 "SPIRVCombinerHelper.h"
+#include "llvm/CodeGen/GlobalISel/GenericMachineInstrs.h"
+#include "llvm/CodeGen/GlobalISel/MIPatternMatch.h"
+#include "llvm/IR/IntrinsicsSPIRV.h"
+#include "llvm/Target/TargetMachine.h"
+
+using namespace llvm;
+using namespace MIPatternMatch;
+
+SPIRVCombinerHelper::SPIRVCombinerHelper(
+ GISelChangeObserver &Observer, MachineIRBuilder &B, bool IsPreLegalize,
+ GISelValueTracking *VT, MachineDominatorTree *MDT, const LegalizerInfo *LI,
+ const SPIRVSubtarget &STI)
+ : CombinerHelper(Observer, B, IsPreLegalize, VT, MDT, LI), STI(STI) {}
+
+/// This match is part of a combine that
+/// rewrites length(X - Y) to distance(X, Y)
+/// (f32 (g_intrinsic length
+/// (g_fsub (vXf32 X) (vXf32 Y))))
+/// ->
+/// (f32 (g_intrinsic distance
+/// (vXf32 X) (vXf32 Y)))
+///
+bool SPIRVCombinerHelper::matchLengthToDistance(MachineInstr &MI) const {
+ if (MI.getOpcode() != TargetOpcode::G_INTRINSIC ||
+ cast<GIntrinsic>(MI).getIntrinsicID() != Intrinsic::spv_length)
+ return false;
+
+ // First operand of MI is `G_INTRINSIC` so start at operand 2.
+ Register SubReg = MI.getOperand(2).getReg();
+ MachineInstr *SubInstr = MRI.getVRegDef(SubReg);
+ if (!SubInstr || SubInstr->getOpcode() != TargetOpcode::G_FSUB)
+ return false;
+
+ return true;
+}
+void SPIRVCombinerHelper::applySPIRVDistance(MachineInstr &MI) const {
+ // Extract the operands for X and Y from the match criteria.
+ Register SubDestReg = MI.getOperand(2).getReg();
+ MachineInstr *SubInstr = MRI.getVRegDef(SubDestReg);
+ Register SubOperand1 = SubInstr->getOperand(1).getReg();
+ Register SubOperand2 = SubInstr->getOperand(2).getReg();
+ Register ResultReg = MI.getOperand(0).getReg();
+
+ Builder.setInstrAndDebugLoc(MI);
+ Builder.buildIntrinsic(Intrinsic::spv_distance, ResultReg)
+ .addUse(SubOperand1)
+ .addUse(SubOperand2);
+
+ MI.eraseFromParent();
+}
diff --git a/llvm/lib/Target/SPIRV/SPIRVCombinerHelper.h b/llvm/lib/Target/SPIRV/SPIRVCombinerHelper.h
new file mode 100644
index 0000000000000..e93b9ec7765d1
--- /dev/null
+++ b/llvm/lib/Target/SPIRV/SPIRVCombinerHelper.h
@@ -0,0 +1,39 @@
+
+//===-- SPIRVCombinerHelper.h -----------------------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+///
+/// This contains common combine transformations that may be used in a combine
+/// pass.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIB_TARGET_SPIRV_SPIRVCOMBINERHELPER_H
+#define LLVM_LIB_TARGET_SPIRV_SPIRVCOMBINERHELPER_H
+
+#include "SPIRVSubtarget.h"
+#include "llvm/CodeGen/GlobalISel/CombinerHelper.h"
+
+namespace llvm {
+class SPIRVCombinerHelper : public CombinerHelper {
+protected:
+ const SPIRVSubtarget &STI;
+
+public:
+ using CombinerHelper::CombinerHelper;
+ SPIRVCombinerHelper(GISelChangeObserver &Observer, MachineIRBuilder &B,
+ bool IsPreLegalize, GISelValueTracking *VT,
+ MachineDominatorTree *MDT, const LegalizerInfo *LI,
+ const SPIRVSubtarget &STI);
+
+ bool matchLengthToDistance(MachineInstr &MI) const;
+ void applySPIRVDistance(MachineInstr &MI) const;
+};
+
+} // end namespace llvm
+
+#endif // LLVM_LIB_TARGET_SPIRV_SPIRVCOMBINERHELPER_H
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
index 83567519355a8..28b36c7f19246 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
@@ -13,24 +13,17 @@
//===----------------------------------------------------------------------===//
#include "SPIRV.h"
-#include "SPIRVTargetMachine.h"
+#include "SPIRVCombinerHelper.h"
#include "llvm/CodeGen/GlobalISel/CSEInfo.h"
#include "llvm/CodeGen/GlobalISel/Combiner.h"
-#include "llvm/CodeGen/GlobalISel/CombinerHelper.h"
#include "llvm/CodeGen/GlobalISel/CombinerInfo.h"
#include "llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h"
#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
#include "llvm/CodeGen/GlobalISel/GISelValueTracking.h"
-#include "llvm/CodeGen/GlobalISel/GenericMachineInstrs.h"
#include "llvm/CodeGen/GlobalISel/MIPatternMatch.h"
-#include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
-#include "llvm/CodeGen/GlobalISel/Utils.h"
#include "llvm/CodeGen/MachineDominators.h"
#include "llvm/CodeGen/MachineFunctionPass.h"
-#include "llvm/CodeGen/MachineRegisterInfo.h"
-#include "llvm/CodeGen/TargetOpcodes.h"
#include "llvm/CodeGen/TargetPassConfig.h"
-#include "llvm/IR/IntrinsicsSPIRV.h"
#define GET_GICOMBINER_DEPS
#include "SPIRVGenPreLegalizeGICombiner.inc"
@@ -47,72 +40,9 @@ namespace {
#include "SPIRVGenPreLegalizeGICombiner.inc"
#undef GET_GICOMBINER_TYPES
-/// This match is part of a combine that
-/// rewrites length(X - Y) to distance(X, Y)
-/// (f32 (g_intrinsic length
-/// (g_fsub (vXf32 X) (vXf32 Y))))
-/// ->
-/// (f32 (g_intrinsic distance
-/// (vXf32 X) (vXf32 Y)))
-///
-bool matchLengthToDistance(MachineInstr &MI, MachineRegisterInfo &MRI) {
- if (MI.getOpcode() != TargetOpcode::G_INTRINSIC ||
- cast<GIntrinsic>(MI).getIntrinsicID() != Intrinsic::spv_length)
- return false;
-
- // First operand of MI is `G_INTRINSIC` so start at operand 2.
- Register SubReg = MI.getOperand(2).getReg();
- MachineInstr *SubInstr = MRI.getVRegDef(SubReg);
- if (!SubInstr || SubInstr->getOpcode() != TargetOpcode::G_FSUB)
- return false;
-
- return true;
-}
-void applySPIRVDistance(MachineInstr &MI, MachineRegisterInfo &MRI,
- MachineIRBuilder &B) {
-
- // Extract the operands for X and Y from the match criteria.
- Register SubDestReg = MI.getOperand(2).getReg();
- MachineInstr *SubInstr = MRI.getVRegDef(SubDestReg);
- Register SubOperand1 = SubInstr->getOperand(1).getReg();
- Register SubOperand2 = SubInstr->getOperand(2).getReg();
-
- // Remove the original `spv_length` instruction.
-
- Register ResultReg = MI.getOperand(0).getReg();
- DebugLoc DL = MI.getDebugLoc();
- MachineBasicBlock &MBB = *MI.getParent();
- MachineBasicBlock::iterator InsertPt = MI.getIterator();
-
- // Build the `spv_distance` intrinsic.
- MachineInstrBuilder NewInstr =
- BuildMI(MBB, InsertPt, DL, B.getTII().get(TargetOpcode::G_INTRINSIC));
- NewInstr
- .addDef(ResultReg) // Result register
- .addIntrinsicID(Intrinsic::spv_distance) // Intrinsic ID
- .addUse(SubOperand1) // Operand X
- .addUse(SubOperand2); // Operand Y
-
- SPIRVGlobalRegistry *GR =
- MI.getMF()->getSubtarget<SPIRVSubtarget>().getSPIRVGlobalRegistry();
- auto RemoveAllUses = [&](Register Reg) {
- SmallVector<MachineInstr *, 4> UsesToErase(
- llvm::make_pointer_range(MRI.use_instructions(Reg)));
-
- // calling eraseFromParent to early invalidates the iterator.
- for (auto *MIToErase : UsesToErase) {
- GR->invalidateMachineInstr(MIToErase);
- MIToErase->eraseFromParent();
- }
- };
- RemoveAllUses(SubDestReg); // remove all uses of FSUB Result
- GR->invalidateMachineInstr(SubInstr);
- SubInstr->eraseFromParent(); // remove FSUB instruction
-}
-
class SPIRVPreLegalizerCombinerImpl : public Combiner {
protected:
- const CombinerHelper Helper;
+ const SPIRVCombinerHelper Helper;
const SPIRVPreLegalizerCombinerImplRuleConfig &RuleConfig;
const SPIRVSubtarget &STI;
@@ -147,7 +77,7 @@ SPIRVPreLegalizerCombinerImpl::SPIRVPreLegalizerCombinerImpl(
const SPIRVSubtarget &STI, MachineDominatorTree *MDT,
const LegalizerInfo *LI)
: Combiner(MF, CInfo, TPC, &VT, CSEInfo),
- Helper(Observer, B, /*IsPreLegalize*/ true, &VT, MDT, LI),
+ Helper(Observer, B, /*IsPreLegalize*/ true, &VT, MDT, LI, STI),
RuleConfig(RuleConfig), STI(STI),
#define GET_GICOMBINER_CONSTRUCTOR_INITS
#include "SPIRVGenPreLegalizeGICombiner.inc"
|
arsenm
approved these changes
Oct 10, 2025
arsenm
reviewed
Oct 10, 2025
|
|
||
| return true; | ||
| } | ||
| void SPIRVCombinerHelper::applySPIRVDistance(MachineInstr &MI) const { |
There was a problem hiding this comment.
You should be able to move this to tablegen
There was a problem hiding this comment.
Can you point me to any examples of how to match a specific intrinsic in tablegen? I can't find a way to specify the spv_distance and spv_length intrinsics in tablegen. This FIXME comment in AMDGPUCombine.td also seems to imply some of the infrastructure for matching intrinsics might be missing.
There was a problem hiding this comment.
I thought the intrinsic thing got fixed, if not it should be.
This should be done in a separate patch anyway.
| /// (f32 (g_intrinsic distance | ||
| /// (vXf32 X) (vXf32 Y))) | ||
| /// | ||
| bool SPIRVCombinerHelper::matchLengthToDistance(MachineInstr &MI) const { |
There was a problem hiding this comment.
This could be moved to tablegen
There was a problem hiding this comment.
Same comment as applySPIRVDistance.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/19577 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/28845 Here is the relevant piece of the build log for the reference |
DharuniRAcharya
pushed a commit
to DharuniRAcharya/llvm-project
that referenced
this pull request
Oct 13, 2025
…iner to use it (llvm#162735) Lots of the code/structure was based off `AMDGPUCombinerHelper`, `AMDGPUPreLegalizerCombiner`, etc. Tasks completed: - Create new `SPIRVCombinerHelper` inheriting from `CombinerHelper` - Move combiner logic in `SPIRVPreLegalizerCombiner` to helper methods in `SPIRVCombinerHelper` - Update `SPIRVPreLegalizerCombiner` to use the new helper class - Simplify `applySPIRVDistance` code
akadutta
pushed a commit
to akadutta/llvm-project
that referenced
this pull request
Oct 14, 2025
…iner to use it (llvm#162735) Lots of the code/structure was based off `AMDGPUCombinerHelper`, `AMDGPUPreLegalizerCombiner`, etc. Tasks completed: - Create new `SPIRVCombinerHelper` inheriting from `CombinerHelper` - Move combiner logic in `SPIRVPreLegalizerCombiner` to helper methods in `SPIRVCombinerHelper` - Update `SPIRVPreLegalizerCombiner` to use the new helper class - Simplify `applySPIRVDistance` code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Lots of the code/structure was based off
AMDGPUCombinerHelper,AMDGPUPreLegalizerCombiner, etc.Tasks completed:
SPIRVCombinerHelperinheriting fromCombinerHelperSPIRVPreLegalizerCombinerto helper methods inSPIRVCombinerHelperSPIRVPreLegalizerCombinerto use the new helper classapplySPIRVDistancecode