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 5 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
5 commits
Select commit Hold shift + click to select a range
be7df59
WIP - debug `matchSelectToFaceForward`
kmpeng Apr 29, 2025
a9459be
finished implementation, modified tests
kmpeng May 13, 2025
28c3910
fixed FIXME in faceforward.ll, edited comments
kmpeng May 14, 2025
e6dfc5b
added scalar implementation and tests
kmpeng May 14, 2025
5cb68c5
partially address PR comments
kmpeng May 16, 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
4 changes: 0 additions & 4 deletions clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,7 @@ template <typename T> constexpr vector<T, 4> lit_impl(T NDotL, T NDotH, T M) {
}

template <typename T> constexpr T faceforward_impl(T N, T I, T Ng) {
#if (__has_builtin(__builtin_spirv_faceforward))
return __builtin_spirv_faceforward(N, I, Ng);
#else
return select(dot(I, Ng) < 0, N, -N);
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be done in this patch


template <typename T> constexpr T ldexp_impl(T X, T Exp) {
Expand Down
56 changes: 40 additions & 16 deletions clang/test/CodeGenHLSL/builtins/faceforward.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@
// CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, half %{{.*}}, half %fneg.i
// CHECK: ret half %hlsl.select.i
// SPVCHECK-LABEL: test_faceforward_half
// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef half @llvm.spv.faceforward.f16(half %{{.*}}, half %{{.*}}, half %{{.*}})
// SPVCHECK: ret half %spv.faceforward.i
// SPVCHECK: %hlsl.dot.i = fmul reassoc nnan ninf nsz arcp afn half %{{.*}}, %{{.*}}
// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn half %{{.*}}
// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, half %{{.*}}, half %fneg.i
// SPVCHECK: ret half %hlsl.select.i
half test_faceforward_half(half N, half I, half Ng) { return faceforward(N, I, Ng); }

// CHECK-LABEL: test_faceforward_half2
Expand All @@ -23,8 +26,11 @@ half test_faceforward_half(half N, half I, half Ng) { return faceforward(N, I, N
// CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x half> %{{.*}}, <2 x half> %fneg.i
// CHECK: ret <2 x half> %hlsl.select.i
// SPVCHECK-LABEL: test_faceforward_half2
// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <2 x half> @llvm.spv.faceforward.v2f16(<2 x half> %{{.*}}, <2 x half> %{{.*}}, <2 x half> %{{.*}})
// SPVCHECK: ret <2 x half> %spv.faceforward.i
// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn half @llvm.spv.fdot.v2f16(<2 x half> %{{.*}}, <2 x half> %{{.*}})
// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <2 x half> %{{.*}}
// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x half> %{{.*}}, <2 x half> %fneg.i
// SPVCHECK: ret <2 x half> %hlsl.select.i
half2 test_faceforward_half2(half2 N, half2 I, half2 Ng) { return faceforward(N, I, Ng); }

// CHECK-LABEL: test_faceforward_half3
Expand All @@ -34,8 +40,11 @@ half2 test_faceforward_half2(half2 N, half2 I, half2 Ng) { return faceforward(N,
// CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x half> %{{.*}}, <3 x half> %fneg.i
// CHECK: ret <3 x half> %hlsl.select.i
// SPVCHECK-LABEL: test_faceforward_half3
// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <3 x half> @llvm.spv.faceforward.v3f16(<3 x half> %{{.*}}, <3 x half> %{{.*}}, <3 x half> %{{.*}})
// SPVCHECK: ret <3 x half> %spv.faceforward.i
// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn half @llvm.spv.fdot.v3f16(<3 x half> %{{.*}}, <3 x half> %{{.*}})
// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <3 x half> %{{.*}}
// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x half> %{{.*}}, <3 x half> %fneg.i
// SPVCHECK: ret <3 x half> %hlsl.select.i
half3 test_faceforward_half3(half3 N, half3 I, half3 Ng) { return faceforward(N, I, Ng); }

// CHECK-LABEL: test_faceforward_half4
Expand All @@ -45,8 +54,11 @@ half3 test_faceforward_half3(half3 N, half3 I, half3 Ng) { return faceforward(N,
// CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x half> %{{.*}}, <4 x half> %fneg.i
// CHECK: ret <4 x half> %hlsl.select.i
// SPVCHECK-LABEL: test_faceforward_half4
// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <4 x half> @llvm.spv.faceforward.v4f16(<4 x half> %{{.*}}, <4 x half> %{{.*}}, <4 x half> %{{.*}})
// SPVCHECK: ret <4 x half> %spv.faceforward.i
// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn half @llvm.spv.fdot.v4f16(<4 x half> %{{.*}}, <4 x half> %{{.*}})
// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <4 x half> %{{.*}}
// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x half> %{{.*}}, <4 x half> %fneg.i
// SPVCHECK: ret <4 x half> %hlsl.select.i
half4 test_faceforward_half4(half4 N, half4 I, half4 Ng) { return faceforward(N, I, Ng); }

// CHECK-LABEL: test_faceforward_float
Expand All @@ -56,8 +68,11 @@ half4 test_faceforward_half4(half4 N, half4 I, half4 Ng) { return faceforward(N,
// CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, float %{{.*}}, float %fneg.i
// CHECK: ret float %hlsl.select.i
// SPVCHECK-LABEL: test_faceforward_float
// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef float @llvm.spv.faceforward.f32(float %{{.*}}, float %{{.*}}, float %{{.*}})
// SPVCHECK: ret float %spv.faceforward.i
// SPVCHECK: %hlsl.dot.i = fmul reassoc nnan ninf nsz arcp afn float %{{.*}}, %{{.*}}
// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn float %{{.*}}
// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, float %{{.*}}, float %fneg.i
// SPVCHECK: ret float %hlsl.select.i
float test_faceforward_float(float N, float I, float Ng) { return faceforward(N, I, Ng); }

// CHECK-LABEL: test_faceforward_float2
Expand All @@ -67,8 +82,11 @@ float test_faceforward_float(float N, float I, float Ng) { return faceforward(N,
// CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x float> %{{.*}}, <2 x float> %fneg.i
// CHECK: ret <2 x float> %hlsl.select.i
// SPVCHECK-LABEL: test_faceforward_float2
// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <2 x float> @llvm.spv.faceforward.v2f32(<2 x float> %{{.*}}, <2 x float> %{{.*}}, <2 x float> %{{.*}})
// SPVCHECK: ret <2 x float> %spv.faceforward.i
// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn float @llvm.spv.fdot.v2f32(<2 x float> %{{.*}}, <2 x float> %{{.*}})
// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <2 x float> %{{.*}}
// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x float> %{{.*}}, <2 x float> %fneg.i
// SPVCHECK: ret <2 x float> %hlsl.select.i
float2 test_faceforward_float2(float2 N, float2 I, float2 Ng) { return faceforward(N, I, Ng); }

// CHECK-LABEL: test_faceforward_float3
Expand All @@ -78,8 +96,11 @@ float2 test_faceforward_float2(float2 N, float2 I, float2 Ng) { return faceforwa
// CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x float> %{{.*}}, <3 x float> %fneg.i
// CHECK: ret <3 x float> %hlsl.select.i
// SPVCHECK-LABEL: test_faceforward_float3
// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <3 x float> @llvm.spv.faceforward.v3f32(<3 x float> %{{.*}}, <3 x float> %{{.*}}, <3 x float> %{{.*}})
// SPVCHECK: ret <3 x float> %spv.faceforward.i
// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn float @llvm.spv.fdot.v3f32(<3 x float> %{{.*}}, <3 x float> %{{.*}})
// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <3 x float> %{{.*}}
// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x float> %{{.*}}, <3 x float> %fneg.i
// SPVCHECK: ret <3 x float> %hlsl.select.i
float3 test_faceforward_float3(float3 N, float3 I, float3 Ng) { return faceforward(N, I, Ng); }

// CHECK-LABEL: test_faceforward_float4
Expand All @@ -89,6 +110,9 @@ float3 test_faceforward_float3(float3 N, float3 I, float3 Ng) { return faceforwa
// CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x float> %{{.*}}, <4 x float> %fneg.i
// CHECK: ret <4 x float> %hlsl.select.i
// SPVCHECK-LABEL: test_faceforward_float4
// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <4 x float> @llvm.spv.faceforward.v4f32(<4 x float> %{{.*}}, <4 x float> %{{.*}}, <4 x float> %{{.*}})
// SPVCHECK: ret <4 x float> %spv.faceforward.i
// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn float @llvm.spv.fdot.v4f32(<4 x float> %{{.*}}, <4 x float> %{{.*}})
// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <4 x float> %{{.*}}
// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x float> %{{.*}}, <4 x float> %fneg.i
// SPVCHECK: ret <4 x float> %hlsl.select.i
float4 test_faceforward_float4(float4 N, float4 I, float4 Ng) { return faceforward(N, I, Ng); }
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 [{ applySPIRVDistance(*${root}, MRI, B); }])
>;

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

def SPIRVPreLegalizerCombiner
: GICombiner<"SPIRVPreLegalizerCombinerImpl",
[vector_length_sub_to_distance_lowering]> {
[vector_length_sub_to_distance_lowering, vector_select_to_faceforward_lowering]> {
let CombineAllMethodName = "tryCombineAllImpl";
}
123 changes: 112 additions & 11 deletions llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,18 @@ namespace {
#include "SPIRVGenPreLegalizeGICombiner.inc"
#undef GET_GICOMBINER_TYPES

static void removeAllUses(Register Reg, MachineRegisterInfo &MRI,
SPIRVGlobalRegistry *GR) {
SmallVector<MachineInstr *, 4> UsesToErase(
llvm::make_pointer_range(MRI.use_instructions(Reg)));

// calling eraseFromParent too early invalidates the iterator.
for (auto *MIToErase : UsesToErase) {
GR->invalidateMachineInstr(MIToErase);
MIToErase->eraseFromParent();
}
}

/// This match is part of a combine that
/// rewrites length(X - Y) to distance(X, Y)
/// (f32 (g_intrinsic length
Expand Down Expand Up @@ -98,21 +110,110 @@ void applySPIRVDistance(MachineInstr &MI, MachineRegisterInfo &MRI,

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
removeAllUses(SubDestReg, MRI, GR); // remove all uses of FSUB Result
GR->invalidateMachineInstr(SubInstr);
SubInstr->eraseFromParent(); // remove FSUB instruction
}

/// This match is part of a combine that
/// rewrites select(fcmp(dot(I, Ng), 0), N, 0 - N) to faceforward(N, I, Ng)
/// (vXf32 (g_select
/// (g_fcmp
/// (g_intrinsic dot(vXf32 I) (vXf32 Ng)
/// 0)
/// (vXf32 N)
/// (vXf32 g_fsub (0) (vXf32 N))))
/// ->
/// (vXf32 (g_intrinsic faceforward
/// (vXf32 N) (vXf32 I) (vXf32 Ng)))
///
/// This only works for Vulkan targets.
///
bool matchSelectToFaceForward(MachineInstr &MI, MachineRegisterInfo &MRI) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be done in tablegen patterns, or at least use mi_match?

const SPIRVSubtarget &ST = MI.getMF()->getSubtarget<SPIRVSubtarget>();
if (!ST.isVulkanEnv())
return false;

if (MI.getOpcode() != TargetOpcode::G_SELECT)
Copy link
Member

@farzonl farzonl May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

can we add something to the match to make sure we are targeting vulkan? You will need to add a test case for openCL like so:

define internal noundef float @faceforward_no_instcombine_float(float noundef %a, float noundef %b, float noundef %c) {
entry:
  ; CHECK: %[[#]] = OpFunction %[[#float_32]] None %[[#]]
  ; CHECK: %[[#arg0:]] = OpFunctionParameter %[[#float_32]]
  ; CHECK: %[[#arg1:]] = OpFunctionParameter %[[#float_32]]
  ; CHECK: %[[#arg2:]] = OpFunctionParameter %[[#float_32]]
  ; CHECK: %[[#]] =  fmul ...
  ; CHECK: %[[#]] =  fcmp ...
  ; CHECK: %[[#]] =  fneg ...
  ; CHECK: %[[#]] =  select ...
  %fmul= fmul float %b, %c
  %fcmp = fcmp olt float %fmul, 0.000000e+00
  %fneg = fneg float %a
  %select = select i1 %fcmp, float %a, float %fneg
  ret float %select
 }

Ie confirm we don't do a pattern replacement.

Copy link
Contributor Author

@kmpeng kmpeng May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Was trying to write this openCL test but ran into this issue for select #135572.

Through manual testing, the check I added for vulkan seems to work.

return false;

// Check if select's condition is a comparison between a dot product and 0.
Register CondReg = MI.getOperand(1).getReg();
MachineInstr *CondInstr = MRI.getVRegDef(CondReg);
if (!CondInstr || CondInstr->getOpcode() != TargetOpcode::G_FCMP)
return false;

Register DotReg = CondInstr->getOperand(2).getReg();
MachineInstr *DotInstr = MRI.getVRegDef(DotReg);
if ((DotInstr->getOpcode() != TargetOpcode::G_INTRINSIC ||
cast<GIntrinsic>(DotInstr)->getIntrinsicID() != Intrinsic::spv_fdot)) {
if (DotInstr->getOpcode() != TargetOpcode::G_FMUL)
return false;
Register DotOperand1 = DotInstr->getOperand(1).getReg();
Register DotOperand2 = DotInstr->getOperand(2).getReg();
if (MRI.getType(DotOperand1).isVector() ||
MRI.getType(DotOperand2).isVector())
return false;
}

Register CondZeroReg = CondInstr->getOperand(3).getReg();
MachineInstr *CondZeroInstr = MRI.getVRegDef(CondZeroReg);
if (CondZeroInstr->getOpcode() != TargetOpcode::G_FCONSTANT ||
!CondZeroInstr->getOperand(1).getFPImm()->isZero())
return false;

// Check if select's false operand is the negation of the true operand.
Register TrueReg = MI.getOperand(2).getReg();
Register FalseReg = MI.getOperand(3).getReg();
MachineInstr *FalseInstr = MRI.getVRegDef(FalseReg);
if (FalseInstr->getOpcode() != TargetOpcode::G_FNEG)
return false;
if (TrueReg != FalseInstr->getOperand(1).getReg())
return false;
Comment on lines +166 to +172
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that the operands be constants like 1 and -1? In that case, this would fail because the False is not a negation. The are many other cases where the optimizer could simplify N and 0-N breaking this pattern matching.

I don't know llvm well enough. Is there a generic way to check if one value is the negation of the other? In other compilers I worked on, you could create a temporary express (a + b) and check if the simplifies folds it to 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was also wondering if this way of checking would be a problem since there's multiple ways to represent 2 operands being negations (another example being G_FSUB). I'm also still learning and have not been able to come up with a generic way to check this. @farzonl Do you have any thoughts?


return true;
}
void applySPIRVFaceForward(MachineInstr &MI, MachineRegisterInfo &MRI,
MachineIRBuilder &B) {

// Extract the operands for N, I, and Ng from the match criteria.
Register CondReg = MI.getOperand(1).getReg();
MachineInstr *CondInstr = MRI.getVRegDef(CondReg);
Register DotReg = CondInstr->getOperand(2).getReg();
MachineInstr *DotInstr = MRI.getVRegDef(DotReg);
Register DotOperand1, DotOperand2;
if (DotInstr->getOpcode() == TargetOpcode::G_FMUL) {
DotOperand1 = DotInstr->getOperand(1).getReg();
DotOperand2 = DotInstr->getOperand(2).getReg();
} else {
DotOperand1 = DotInstr->getOperand(2).getReg();
DotOperand2 = DotInstr->getOperand(3).getReg();
}
Register TrueReg = MI.getOperand(2).getReg();

// Remove the original `select` instruction.
Register ResultReg = MI.getOperand(0).getReg();
DebugLoc DL = MI.getDebugLoc();
MachineBasicBlock &MBB = *MI.getParent();
MachineBasicBlock::iterator InsertPt = MI.getIterator();

// Build the `spv_faceforward` intrinsic.
MachineInstrBuilder NewInstr =
BuildMI(MBB, InsertPt, DL, B.getTII().get(TargetOpcode::G_INTRINSIC));
NewInstr
.addDef(ResultReg) // Result register
.addIntrinsicID(Intrinsic::spv_faceforward) // Intrinsic ID
.addUse(TrueReg) // Operand N
.addUse(DotOperand1) // Operand I
.addUse(DotOperand2); // Operand Ng

SPIRVGlobalRegistry *GR =
MI.getMF()->getSubtarget<SPIRVSubtarget>().getSPIRVGlobalRegistry();
removeAllUses(CondReg, MRI, GR); // remove all uses of FCMP Result
GR->invalidateMachineInstr(CondInstr);
CondInstr->eraseFromParent(); // remove FCMP instruction
}

class SPIRVPreLegalizerCombinerImpl : public Combiner {
protected:
const CombinerHelper Helper;
Expand Down
65 changes: 65 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,65 @@
# RUN: llc -verify-machineinstrs -O0 -mtriple spirv-unknown-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)
Copy link
Member

@farzonl farzonl May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is probably my fault, I don't think I did enought in the distance ticket to test it properly. But in length to distance we were replacing one intrinsic with another so I think it reuses the intrinsic side effects. In this case we are creating an intrinsic out of nothing should we be wrapping the intrinsic in G_INTRINSIC_W_SIDE_EFFECTS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really familiar with what G_INTRINSIC_W_SIDE_EFFECTS is. From my understanding, the spirv faceforward intrinsic is treated as a pure mathematical operation that doesn't have side effects (as it has the IntrNoMem property), so this shouldn't be needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to introduce side effects out of nothing (and the G_INTRINSIC* opcode used should be consistent with the defined attributes for the underlying intrinsic)

; 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
OpDecorate %5:iid(s64), 41, 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 13428, 0
%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>)

Loading