[SPIRV] Add PreLegalizer pattern matching for `faceforward` #139959

kmpeng · 2025-05-14T20:32:44Z

Tasks completed:

Pattern matchselect(fcmp(dot(p2, p3), 0), p1, 0 - p1) to faceforward(p1, p2, p3)
Remove HLSL faceforward intrinsic's SPIR-V fast path
Add pattern matching tests to prelegalizercombiner-select-to-faceforward.mir and faceforward.ll
Add CL extension error test llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll

Closes #137255.

llvmbot · 2025-05-14T20:33:15Z

@llvm/pr-subscribers-llvm-globalisel
@llvm/pr-subscribers-backend-x86

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

Author: Kaitlin Peng (kmpeng)

Changes

Tasks completed:

instCombine select(fcmp(dot(p2, p3), 0), p1, 0 - p1) to faceforward(p1, p2, p3)
Remove HLSL faceforward intrinsic's SPIR-V fast path
Add instCombine tests to prelegalizercombiner-select-to-faceforward.mir and faceforward.ll
Add CL extension error test llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll

Closes #137255.

Patch is 25.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139959.diff

7 Files Affected:

(modified) clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h (-4)
(modified) clang/test/CodeGenHLSL/builtins/faceforward.hlsl (+40-16)
(modified) llvm/lib/Target/SPIRV/SPIRVCombine.td (+8-1)
(modified) llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp (+100-11)
(added) llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir (+63)
(modified) llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll (+42-8)
(added) llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll (+13)

diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h b/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
index 4eb7b8f45c85a..e1996fd3e30ed 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
@@ -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
 }
 
 template <typename T> constexpr T ldexp_impl(T X, T Exp) {
diff --git a/clang/test/CodeGenHLSL/builtins/faceforward.hlsl b/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
index d2ece57aba4ae..f12f75f384964 100644
--- a/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
+++ b/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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); }
diff --git a/llvm/lib/Target/SPIRV/SPIRVCombine.td b/llvm/lib/Target/SPIRV/SPIRVCombine.td
index 6f726e024de52..78a57146e61d2 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCombine.td
+++ b/llvm/lib/Target/SPIRV/SPIRVCombine.td
@@ -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";
 }
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
index c96ee6b02491a..82c5cbf75b849 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
@@ -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
@@ -98,21 +110,98 @@ 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)))
+///
+bool matchSelectToFaceForward(MachineInstr &MI, MachineRegisterInfo &MRI) {
+  if (MI.getOpcode() != TargetOpcode::G_SELECT)
+    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_FMUL &&
+      (DotInstr->getOpcode() != TargetOpcode::G_INTRINSIC ||
+       cast<GIntrinsic>(DotInstr)->getIntrinsicID() != Intrinsic::spv_fdot))
+    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;
+
+  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;
diff --git a/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir b/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir
new file mode 100644
index 0000000000000..08f03942460ba
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir
@@ -0,0 +1,63 @@
+# 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)
+    %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>)
+    %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>)
+      
\ No newline at end of file
diff --git a/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll b/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
index 22742169506f2..6811f95bbab17 100644
--- a/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
+++ b/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
@@ -1,7 +1,5 @@
-; RUN: llc -O0 -verify-machineinstrs -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
-; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val --target-env spv1.4 %}
-
-; FIXME(#136344): Change --target-env to vulkan1.3 and update this test accordingly once the issue is resolved.
+; RUN: llc -O0 -verify-machineinstrs -mtriple=spirv-vulkan1.3-unknown %s -o...
[truncated]

llvmbot · 2025-05-14T20:33:15Z

@llvm/pr-subscribers-hlsl

Author: Kaitlin Peng (kmpeng)

Changes

Tasks completed:

instCombine select(fcmp(dot(p2, p3), 0), p1, 0 - p1) to faceforward(p1, p2, p3)
Remove HLSL faceforward intrinsic's SPIR-V fast path
Add instCombine tests to prelegalizercombiner-select-to-faceforward.mir and faceforward.ll
Add CL extension error test llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll

Closes #137255.

Patch is 25.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139959.diff

7 Files Affected:

(modified) clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h (-4)
(modified) clang/test/CodeGenHLSL/builtins/faceforward.hlsl (+40-16)
(modified) llvm/lib/Target/SPIRV/SPIRVCombine.td (+8-1)
(modified) llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp (+100-11)
(added) llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir (+63)
(modified) llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll (+42-8)
(added) llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll (+13)

diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h b/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
index 4eb7b8f45c85a..e1996fd3e30ed 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
@@ -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
 }
 
 template <typename T> constexpr T ldexp_impl(T X, T Exp) {
diff --git a/clang/test/CodeGenHLSL/builtins/faceforward.hlsl b/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
index d2ece57aba4ae..f12f75f384964 100644
--- a/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
+++ b/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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); }
diff --git a/llvm/lib/Target/SPIRV/SPIRVCombine.td b/llvm/lib/Target/SPIRV/SPIRVCombine.td
index 6f726e024de52..78a57146e61d2 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCombine.td
+++ b/llvm/lib/Target/SPIRV/SPIRVCombine.td
@@ -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";
 }
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
index c96ee6b02491a..82c5cbf75b849 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
@@ -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
@@ -98,21 +110,98 @@ 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)))
+///
+bool matchSelectToFaceForward(MachineInstr &MI, MachineRegisterInfo &MRI) {
+  if (MI.getOpcode() != TargetOpcode::G_SELECT)
+    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_FMUL &&
+      (DotInstr->getOpcode() != TargetOpcode::G_INTRINSIC ||
+       cast<GIntrinsic>(DotInstr)->getIntrinsicID() != Intrinsic::spv_fdot))
+    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;
+
+  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;
diff --git a/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir b/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir
new file mode 100644
index 0000000000000..08f03942460ba
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir
@@ -0,0 +1,63 @@
+# 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)
+    %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>)
+    %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>)
+      
\ No newline at end of file
diff --git a/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll b/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
index 22742169506f2..6811f95bbab17 100644
--- a/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
+++ b/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
@@ -1,7 +1,5 @@
-; RUN: llc -O0 -verify-machineinstrs -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
-; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val --target-env spv1.4 %}
-
-; FIXME(#136344): Change --target-env to vulkan1.3 and update this test accordingly once the issue is resolved.
+; RUN: llc -O0 -verify-machineinstrs -mtriple=spirv-vulkan1.3-unknown %s -o...
[truncated]

arsenm

Description mentions instcombine but this has nothing to do with instcombine

arsenm · 2025-05-14T21:32:26Z

clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h

  return select(dot(I, Ng) < 0, N, -N);
-#endif
 }


Should not be done in this patch

llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll

llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir

farzonl · 2025-05-15T20:48:48Z

llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir

+    ; 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)


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?

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?

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)

llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir

farzonl · 2025-05-15T20:53:46Z

llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp

+///             (vXf32 N) (vXf32 I) (vXf32 Ng)))
+///
+bool matchSelectToFaceForward(MachineInstr &MI, MachineRegisterInfo &MRI) {
+  if (MI.getOpcode() != TargetOpcode::G_SELECT)


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.

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.

arsenm

Description should not mention instcombine, instcombine is not related

s-perron

I have a minor concern about how specific the N and 0-N checks are. Otherwise this looks good to me.

s-perron · 2025-05-16T14:49:22Z

llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp

+
+  Register DotReg = CondInstr->getOperand(2).getReg();
+  MachineInstr *DotInstr = MRI.getVRegDef(DotReg);
+  if (DotInstr->getOpcode() != TargetOpcode::G_FMUL &&


I want to double check that G_FMUL cannot apply to vectors of floating point values. If it does, this will not be equivalent to a dot product.

You're right, this would pose a problem. Made an update that checks that the G_FMUL operands are not vectors. Let me know if that looks good.

s-perron · 2025-05-16T15:00:35Z

llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp

+  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;


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.

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?

arsenm · 2025-05-16T15:14:13Z

llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp

+///   (vXf32 (g_intrinsic faceforward
+///             (vXf32 N) (vXf32 I) (vXf32 Ng)))
+///
+bool matchSelectToFaceForward(MachineInstr &MI, MachineRegisterInfo &MRI) {


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

kmpeng added 4 commits May 13, 2025 16:16

WIP - debug matchSelectToFaceForward

be7df59

finished implementation, modified tests

a9459be

fixed FIXME in faceforward.ll, edited comments

28c3910

added scalar implementation and tests

e6dfc5b

llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics llvm:globalisel HLSL HLSL Language Support backend:SPIR-V labels May 14, 2025

arsenm reviewed May 14, 2025

View reviewed changes

clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h

return select(dot(I, Ng) < 0, N, -N);

#endif

}

Copy link

Contributor

arsenm May 14, 2025

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

farzonl reviewed May 15, 2025

View reviewed changes

llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll Show resolved Hide resolved

farzonl reviewed May 15, 2025

View reviewed changes

llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir Outdated Show resolved Hide resolved

farzonl reviewed May 15, 2025

View reviewed changes

llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir Show resolved Hide resolved

farzonl reviewed May 15, 2025

View reviewed changes

arsenm reviewed May 16, 2025

View reviewed changes

s-perron reviewed May 16, 2025

View reviewed changes

arsenm reviewed May 16, 2025

View reviewed changes

kmpeng changed the title ~~[SPIRV] Add PreLegalizer instCombine for faceforward~~ [SPIRV] Add PreLegalizer pattern matching for faceforward May 16, 2025

partially address PR comments

5cb68c5

[SPIRV] Add PreLegalizer pattern matching for faceforward #139959

Are you sure you want to change the base?

[SPIRV] Add PreLegalizer pattern matching for faceforward #139959

Uh oh!

Conversation

kmpeng commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 14, 2025

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm May 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

farzonl May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmpeng May 16, 2025

Choose a reason for hiding this comment

Uh oh!

arsenm May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

farzonl May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmpeng May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

s-perron left a comment

Choose a reason for hiding this comment

Uh oh!

s-perron May 16, 2025

Choose a reason for hiding this comment

Uh oh!

kmpeng May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-perron May 16, 2025

Choose a reason for hiding this comment

Uh oh!

kmpeng May 17, 2025

Choose a reason for hiding this comment

Uh oh!

arsenm May 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[SPIRV] Add PreLegalizer pattern matching for `faceforward` #139959

[SPIRV] Add PreLegalizer pattern matching for `faceforward` #139959

kmpeng commented May 14, 2025 •

edited

Loading

llvmbot commented May 14, 2025 •

edited

Loading

farzonl May 15, 2025 •

edited

Loading

farzonl May 15, 2025 •

edited

Loading

kmpeng May 19, 2025 •

edited

Loading

kmpeng May 16, 2025 •

edited

Loading