Skip to content

[SPIRV] Add PreLegalizer pattern matching for faceforward #139959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[SPIRV] Add PreLegalizer pattern matching for faceforward #139959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
910cb2c
WIP - debug `matchSelectToFaceForward`
kmpeng Apr 29, 2025
4a45d79
finished implementation, modified tests
kmpeng May 13, 2025
f284a0b
fixed FIXME in faceforward.ll, edited comments
kmpeng May 14, 2025
154c8df
added scalar implementation and tests
kmpeng May 14, 2025
48d3ccb
partially address PR comments
kmpeng May 16, 2025
2a65175
add opencl test, fix .mir test, fix vulkan check in combiner
kmpeng Sep 12, 2025
fa580b9
undo frontend changes, change pattern matching to use mi_match, add c…
kmpeng Sep 24, 2025
900b7bb
edit true/false operand matching to work more robustly with constant …
kmpeng Sep 24, 2025
04528a6
change vulkan target check, use MIRBuilder
kmpeng Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion llvm/lib/Target/SPIRV/SPIRVCombine.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,15 @@ def vector_length_sub_to_distance_lowering : GICombineRule <
(apply [{ Helper.applySPIRVDistance(*${root}); }])
>;

def vector_select_to_faceforward_lowering : GICombineRule <
(defs root:$root),
(match (wip_match_opcode G_SELECT):$root,
[{ return Helper.matchSelectToFaceForward(*${root}); }]),
(apply [{ Helper.applySPIRVFaceForward(*${root}); }])
>;

def SPIRVPreLegalizerCombiner
: GICombiner<"SPIRVPreLegalizerCombinerImpl",
[vector_length_sub_to_distance_lowering]> {
[vector_length_sub_to_distance_lowering, vector_select_to_faceforward_lowering]> {
let CombineAllMethodName = "tryCombineAllImpl";
}
103 changes: 103 additions & 0 deletions llvm/lib/Target/SPIRV/SPIRVCombinerHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,106 @@ void SPIRVCombinerHelper::applySPIRVDistance(MachineInstr &MI) const {

MI.eraseFromParent();
}

/// This match is part of a combine that
/// rewrites select(fcmp(dot(I, Ng), 0), N, -N) to faceforward(N, I, Ng)
/// (vXf32 (g_select
/// (g_fcmp
/// (g_intrinsic dot(vXf32 I) (vXf32 Ng)
/// 0)
/// (vXf32 N)
/// (vXf32 g_fneg (vXf32 N))))
/// ->
/// (vXf32 (g_intrinsic faceforward
/// (vXf32 N) (vXf32 I) (vXf32 Ng)))
///
/// This only works for Vulkan targets.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// This only works for Vulkan targets.
/// This only works for Vulkan shader targets.

///
bool SPIRVCombinerHelper::matchSelectToFaceForward(MachineInstr &MI) const {
if (!STI.isShader())
return false;

// Match overall select pattern.
Register CondReg, TrueReg, FalseReg;
if (!mi_match(MI.getOperand(0).getReg(), MRI,
m_GISelect(m_Reg(CondReg), m_Reg(TrueReg), m_Reg(FalseReg))))
return false;

// Match the FCMP condition.
Register DotReg, CondZeroReg;
CmpInst::Predicate Pred;
if (!mi_match(CondReg, MRI,
m_GFCmp(m_Pred(Pred), m_Reg(DotReg), m_Reg(CondZeroReg))) ||
Pred != CmpInst::FCMP_OLT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be overly specialized to ordered and less than? What about these 16 other fcmp condition codes? https://llvm.org/docs/LangRef.html#fcmp-instruction

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my concern is what if an optimization pass comes along and rewrites our faceforward implementation and the condition changes for the comparison. How robust are we to handling something like this?

return false;

// Check if FCMP is a comparison between a dot product and 0.
MachineInstr *DotInstr = MRI.getVRegDef(DotReg);
if (DotInstr->getOpcode() != TargetOpcode::G_INTRINSIC ||
cast<GIntrinsic>(DotInstr)->getIntrinsicID() != Intrinsic::spv_fdot) {
Register DotOperand1, DotOperand2;
// Check for scalar dot product.
if (!mi_match(DotReg, MRI,
m_GFMul(m_Reg(DotOperand1), m_Reg(DotOperand2))) ||
!MRI.getType(DotOperand1).isScalar() ||
!MRI.getType(DotOperand2).isScalar())
return false;
}

const ConstantFP *ZeroVal;
if (!mi_match(CondZeroReg, MRI, m_GFCst(ZeroVal)) || !ZeroVal->isZero())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think APFloats can be -0.0 and 0.0. Would be good to make sure in a test case that both work to get past this condition.

return false;

// Check if select's false operand is the negation of the true operand.
auto AreNegatedConstants = [&](Register TrueReg, Register FalseReg) {
const ConstantFP *TrueVal, *FalseVal;
if (!mi_match(TrueReg, MRI, m_GFCst(TrueVal)) ||
!mi_match(FalseReg, MRI, m_GFCst(FalseVal)))
return false;
APFloat TrueValNegated = TrueVal->getValue();
TrueValNegated.changeSign();
return FalseVal->getValue().compare(TrueValNegated) == APFloat::cmpEqual;
};

if (!mi_match(FalseReg, MRI, m_GFNeg(m_SpecificReg(TrueReg))) &&
!mi_match(TrueReg, MRI, m_GFNeg(m_SpecificReg(FalseReg)))) {
// Check if they're constant opposites.
MachineInstr *TrueInstr = MRI.getVRegDef(TrueReg);
MachineInstr *FalseInstr = MRI.getVRegDef(FalseReg);
if (TrueInstr->getOpcode() == TargetOpcode::G_BUILD_VECTOR &&
FalseInstr->getOpcode() == TargetOpcode::G_BUILD_VECTOR &&
Comment on lines +127 to +128
Copy link
Member

@farzonl farzonl Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the negated-constant handling too narrow? The “opposites” check handles scalars and G_BUILD_VECTOR of literals, but it isn'y clear if this would catch common splat forms (e.g., splat from G_FNEG of a scalar, insert/extract builds, or fmul-by--1.0). Will all the splat of scalar to vector be converted to vectors by the time we get to the SPIRV backend?

TrueInstr->getNumOperands() == FalseInstr->getNumOperands()) {
for (unsigned I = 1; I < TrueInstr->getNumOperands(); ++I)
if (!AreNegatedConstants(TrueInstr->getOperand(I).getReg(),
FalseInstr->getOperand(I).getReg()))
return false;
} else if (!AreNegatedConstants(TrueReg, FalseReg))
return false;
}

return true;
}

void SPIRVCombinerHelper::applySPIRVFaceForward(MachineInstr &MI) const {
// Extract the operands for N, I, and Ng from the match criteria.
Register CondReg, TrueReg, DotReg, DotOperand1, DotOperand2;
if (!mi_match(MI.getOperand(0).getReg(), MRI,
m_GISelect(m_Reg(CondReg), m_Reg(TrueReg), m_Reg())))
return;
if (!mi_match(CondReg, MRI, m_GFCmp(m_Pred(), m_Reg(DotReg), m_Reg())))
return;
MachineInstr *DotInstr = MRI.getVRegDef(DotReg);
if (!mi_match(DotReg, MRI, m_GFMul(m_Reg(DotOperand1), m_Reg(DotOperand2)))) {
DotOperand1 = DotInstr->getOperand(2).getReg();
DotOperand2 = DotInstr->getOperand(3).getReg();
}
Comment on lines +150 to +153
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we do mi_match both in matchSelectToFaceForward and in applySPIRVFaceForward? I figured apply would be just do the transformation. The only matching i'm not seeing in apply is the dot product intrinsic matching.


Register ResultReg = MI.getOperand(0).getReg();
Builder.setInstrAndDebugLoc(MI);
Builder.buildIntrinsic(Intrinsic::spv_faceforward, ResultReg)
.addUse(TrueReg) // N
.addUse(DotOperand1) // I
.addUse(DotOperand2); // Ng

MI.eraseFromParent();
}
2 changes: 2 additions & 0 deletions llvm/lib/Target/SPIRV/SPIRVCombinerHelper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ class SPIRVCombinerHelper : public CombinerHelper {

bool matchLengthToDistance(MachineInstr &MI) const;
void applySPIRVDistance(MachineInstr &MI) const;
bool matchSelectToFaceForward(MachineInstr &MI) const;
void applySPIRVFaceForward(MachineInstr &MI) const;
};

} // end namespace llvm
Expand Down
133 changes: 133 additions & 0 deletions .../test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
# RUN: llc -verify-machineinstrs -O0 -mtriple spirv-vulkan1.3-unknown -run-pass=spirv-prelegalizer-combiner %s -o - | FileCheck %s
# REQUIRES: asserts
---
name: faceforward_instcombine_float
tracksRegLiveness: true
legalized: true
body: |
bb.1.entry:
; CHECK-LABEL: name: faceforward_instcombine_float
; CHECK-NOT: %9:_(s32) = G_FCONSTANT float 0.000000e+00
; CHECK-NOT: %8:_(s32) = G_FMUL %1:fid, %2:fid
; CHECK-NOT: %10:_(s1) = G_FCMP floatpred(olt), %8:_(s32), %9:_
; CHECK-NOT: %11:_(s32) = G_FNEG %0:fid
; CHECK-NOT: %12:id(s32) = G_SELECT %10:_(s1), %0:fid, %11:_
; CHECK: %10:id(s32) = G_INTRINSIC intrinsic(@llvm.spv.faceforward), %2(s32), %3(s32), %4(s32)
; CHECK: OpReturnValue %10(s32)
%3:type(s64) = OpTypeFloat 32
%5:type(s64) = OpTypeFunction %3:type(s64), %3:type(s64), %3:type(s64), %3:type(s64)
OpName %0:fid(s32), 97
OpName %1:fid(s32), 98
OpName %2:fid(s32), 99
%4:iid(s64) = OpFunction %3:type(s64), 0, %5:type(s64)
%0:fid(s32) = OpFunctionParameter %3:type(s64)
%1:fid(s32) = OpFunctionParameter %3:type(s64)
%2:fid(s32) = OpFunctionParameter %3:type(s64)
OpName %4:iid(s64), 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 116
%9:_(s32) = G_FCONSTANT float 0.000000e+00
%8:_(s32) = G_FMUL %1:fid, %2:fid
%10:_(s1) = G_FCMP floatpred(olt), %8:_(s32), %9:_
%11:_(s32) = G_FNEG %0:fid
%12:id(s32) = G_SELECT %10:_(s1), %0:fid, %11:_
OpReturnValue %12:id(s32)
---
name: faceforward_instcombine_float4
tracksRegLiveness: true
legalized: true
body: |
bb.1.entry:
; CHECK-LABEL: name: faceforward_instcombine_float4
; CHECK-NOT: %10:_(s32) = G_FCONSTANT float 0.000000e+00
; CHECK-NOT: %9:_(s32) = G_INTRINSIC intrinsic(@llvm.spv.fdot), %1:vfid(<4 x s32>), %2:vfid(<4 x s32>)
; CHECK-NOT: %11:_(s1) = G_FCMP floatpred(olt), %9:_(s32), %10:_
; CHECK-NOT: %12:_(<4 x s32>) = G_FNEG %0:vfid
; CHECK-NOT: %13:id(<4 x s32>) = G_SELECT %11:_(s1), %0:vfid, %12:_
; CHECK: %11:id(<4 x s32>) = G_INTRINSIC intrinsic(@llvm.spv.faceforward), %3(<4 x s32>), %4(<4 x s32>), %5(<4 x s32>)
; CHECK: OpReturnValue %11(<4 x s32>)
%4:type(s64) = OpTypeVector %3:type(s64), 4
%6:type(s64) = OpTypeFunction %4:type(s64), %4:type(s64), %4:type(s64), %4:type(s64)
%3:type(s64) = OpTypeFloat 32
OpName %0:vfid(<4 x s32>), 97
OpName %1:vfid(<4 x s32>), 98
OpName %2:vfid(<4 x s32>), 99
%5:iid(s64) = OpFunction %4:type(s64), 0, %6:type(s64)
%0:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
%1:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
%2:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
OpName %5:iid(s64), 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 13428
%10:_(s32) = G_FCONSTANT float 0.000000e+00
%9:_(s32) = G_INTRINSIC intrinsic(@llvm.spv.fdot), %1:vfid(<4 x s32>), %2:vfid(<4 x s32>)
%11:_(s1) = G_FCMP floatpred(olt), %9:_(s32), %10:_
%12:_(<4 x s32>) = G_FNEG %0:vfid
%13:id(<4 x s32>) = G_SELECT %11:_(s1), %0:vfid, %12:_
OpReturnValue %13:id(<4 x s32>)
---
name: faceforward_instcombine_float_constants
tracksRegLiveness: true
legalized: true
body: |
bb.1.entry:
; CHECK-LABEL: name: faceforward_instcombine_float_constants
; CHECK-NOT: %9:_(s32) = G_FCONSTANT float 0.000000e+00
; CHECK-NOT: %13:_(s32) = G_FCONSTANT float -1.000000e+00
; CHECK-NOT: %8:_(s32) = G_FMUL %1:fid, %2:fid
; CHECK-NOT: %10:_(s1) = G_FCMP floatpred(olt), %8:_(s32), %9:_
; CHECK-NOT: %11:id(s32) = G_SELECT %10:_(s1), %12:_, %13:_
; CHECK: %7:_(s32) = G_FCONSTANT float 1.000000e+00
; CHECK: %11:id(s32) = G_INTRINSIC intrinsic(@llvm.spv.faceforward), %7(s32), %3(s32), %4(s32)
; CHECK: OpReturnValue %11(s32)
%3:type(s64) = OpTypeFloat 32
%5:type(s64) = OpTypeFunction %3:type(s64), %3:type(s64), %3:type(s64), %3:type(s64)
OpName %0:fid(s32), 97
OpName %1:fid(s32), 98
OpName %2:fid(s32), 99
%4:iid(s64) = OpFunction %3:type(s64), 0, %5:type(s64)
%0:fid(s32) = OpFunctionParameter %3:type(s64)
%1:fid(s32) = OpFunctionParameter %3:type(s64)
%2:fid(s32) = OpFunctionParameter %3:type(s64)
OpName %4:iid(s64), 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 1868783476, 1635021678, 7566446
%9:_(s32) = G_FCONSTANT float 0.000000e+00
%12:_(s32) = G_FCONSTANT float 1.000000e+00
%13:_(s32) = G_FCONSTANT float -1.000000e+00
%8:_(s32) = G_FMUL %1:fid, %2:fid
%10:_(s1) = G_FCMP floatpred(olt), %8:_(s32), %9:_
%11:id(s32) = G_SELECT %10:_(s1), %12:_, %13:_
OpReturnValue %11:id(s32)
---
name: faceforward_instcombine_float4_constants
tracksRegLiveness: true
legalized: true
body: |
bb.1.entry:
; CHECK-LABEL: name: faceforward_instcombine_float4
; CHECK-NOT: %10:_(s32) = G_FCONSTANT float 0.000000e+00
; CHECK-NOT: %16:_(s32) = G_FCONSTANT float -1.000000e+00
; CHECK-NOT: %15:_(<4 x s32>) = G_BUILD_VECTOR %16:_(s32), %16:_(s32), %16:_(s32), %16:_(s32)
; CHECK-NOT: %9:_(s32) = G_INTRINSIC intrinsic(@llvm.spv.fdot), %1:vfid(<4 x s32>), %2:vfid(<4 x s32>)
; CHECK-NOT: %11:_(s1) = G_FCMP floatpred(olt), %9:_(s32), %10:_
; CHECK-NOT: %12:id(<4 x s32>) = G_SELECT %11:_(s1), %13:_, %15:_
; CHECK: %8:_(s32) = G_FCONSTANT float 1.000000e+00
; CHECK: %9:_(<4 x s32>) = G_BUILD_VECTOR %8(s32), %8(s32), %8(s32), %8(s32)
; CHECK: %14:id(<4 x s32>) = G_INTRINSIC intrinsic(@llvm.spv.faceforward), %9(<4 x s32>), %4(<4 x s32>), %5(<4 x s32>)
; CHECK: OpReturnValue %14(<4 x s32>)
%4:type(s64) = OpTypeVector %3:type(s64), 4
%6:type(s64) = OpTypeFunction %4:type(s64), %4:type(s64), %4:type(s64), %4:type(s64)
%3:type(s64) = OpTypeFloat 32
OpName %0:vfid(<4 x s32>), 97
OpName %1:vfid(<4 x s32>), 98
OpName %2:vfid(<4 x s32>), 99
%5:iid(s64) = OpFunction %4:type(s64), 0, %6:type(s64)
%0:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
%1:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
%2:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
OpName %5:iid(s64), 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 1667183732, 1953721967, 1937010273, 0
%10:_(s32) = G_FCONSTANT float 0.000000e+00
%14:_(s32) = G_FCONSTANT float 1.000000e+00
%13:_(<4 x s32>) = G_BUILD_VECTOR %14:_(s32), %14:_(s32), %14:_(s32), %14:_(s32)
%16:_(s32) = G_FCONSTANT float -1.000000e+00
%15:_(<4 x s32>) = G_BUILD_VECTOR %16:_(s32), %16:_(s32), %16:_(s32), %16:_(s32)
%9:_(s32) = G_INTRINSIC intrinsic(@llvm.spv.fdot), %1:vfid(<4 x s32>), %2:vfid(<4 x s32>)
%11:_(s1) = G_FCMP floatpred(olt), %9:_(s32), %10:_
%12:id(<4 x s32>) = G_SELECT %11:_(s1), %13:_, %15:_
OpReturnValue %12:id(<4 x s32>)

Loading