Skip to content

Conversation

@kmpeng
Copy link
Contributor

@kmpeng kmpeng commented May 14, 2025

Tasks completed:

  • Pattern matchselect(fcmp(dot(p2, p3), 0), p1, -p1) to faceforward(p1, p2, p3)
  • 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
  • Add CL extension test for no pattern matching in llvm/test/CodeGen/SPIRV/opencl/faceforward.ll

Closes #137255.

@llvmbot 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
@llvmbot
Copy link
Member

llvmbot commented May 14, 2025

@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
Copy link
Member

llvmbot commented May 14, 2025

@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]

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

Description mentions instcombine but this has nothing to do with instcombine

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

Description should not mention instcombine, instcombine is not related

Copy link
Contributor

@s-perron s-perron left a comment

Choose a reason for hiding this comment

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

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

@kmpeng kmpeng changed the title [SPIRV] Add PreLegalizer instCombine for faceforward [SPIRV] Add PreLegalizer pattern matching for faceforward May 16, 2025
@kmpeng kmpeng force-pushed the faceforward-patternmatch branch from 5cb68c5 to 6d19a11 Compare September 12, 2025 21:44
@kmpeng kmpeng force-pushed the faceforward-patternmatch branch 2 times, most recently from 67cad12 to 6868f35 Compare September 24, 2025 21:16
@kmpeng kmpeng force-pushed the faceforward-patternmatch branch from 64df069 to 900b7bb Compare October 13, 2025 18:44
@kmpeng kmpeng requested a review from arsenm October 14, 2025 18:02
@kmpeng kmpeng force-pushed the faceforward-patternmatch branch from 4871e02 to 824225d Compare October 31, 2025 03:21
Copy link
Member

@farzonl farzonl left a comment

Choose a reason for hiding this comment

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

LGTM, wait for other feedback though.

@kmpeng kmpeng force-pushed the faceforward-patternmatch branch from 824225d to eb3994f Compare November 20, 2025 21:48
@kmpeng
Copy link
Contributor Author

kmpeng commented Nov 20, 2025

@arsenm @s-perron Would either of you like to have another look? It's ready to go in our eyes.

@github-actions
Copy link

🐧 Linux x64 Test Results

  • 186435 tests passed
  • 4864 tests skipped

Copy link
Contributor

@s-perron s-perron left a comment

Choose a reason for hiding this comment

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

LGTM

@kmpeng kmpeng merged commit 4877c59 into llvm:main Nov 25, 2025
11 checks passed
augusto2112 pushed a commit to augusto2112/llvm-project that referenced this pull request Dec 3, 2025
)

Tasks completed:
- Pattern match`select(fcmp(dot(p2, p3), 0), p1, -p1)` to
`faceforward(p1, p2, p3)`
- 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`
- Add CL extension test for no pattern matching in
`llvm/test/CodeGen/SPIRV/opencl/faceforward.ll`

Closes llvm#137255.
kcloudy0717 pushed a commit to kcloudy0717/llvm-project that referenced this pull request Dec 4, 2025
)

Tasks completed:
- Pattern match`select(fcmp(dot(p2, p3), 0), p1, -p1)` to
`faceforward(p1, p2, p3)`
- 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`
- Add CL extension test for no pattern matching in
`llvm/test/CodeGen/SPIRV/opencl/faceforward.ll`

Closes llvm#137255.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SPIRV] Add PreLegalizer pattern matching for faceforward GL extension

5 participants