[DEBUG] Revert "Enable SPV_INTEL_fp_fast_math_mode (#4058)"

[anmyachev]

This reverts commit 353d6ff.

New CI link:

• E2E before rebase on main: https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15567222381
• Inductor after rebase: https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15630616934 (failed, but there is a regression on main)
• New run: https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15638597833 (passed)

Before rebasing on main: there was one failure: https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15617981986/job/43995909816?pr=4473

[anmyachev]

@whitneywhtsang could you remind me how to disable this mode using env var?

[whitneywhtsang]

@whitneywhtsang could you remind me how to disable this mode using env var?

By default fast math is not enabled, but allow contract is, it can be disabled by TRITON_INTEL_FAST_MATH=0 or TRITON_DEFAULT_FP_FUSION=0.

[whitneywhtsang]

Instead of reverting the SPV extension, can we do the change below?

diff --git a/third_party/intel/triton_xpu.cc b/third_party/intel/triton_xpu.cc
index 908d57c2e6..286eadd7f5 100644
--- a/third_party/intel/triton_xpu.cc
+++ b/third_party/intel/triton_xpu.cc
@@ -296,7 +296,7 @@ void init_triton_intel(py::module &&m) {
         if (auto *op = dyn_cast<FPMathOperator>(&inst)) {
           FastMathFlags FMF;
           // Default to allow contract when default fp fusion is not disabled.
-          if ((!enableFpFusion.has_value() || enableFpFusion.value()) &&
+          if ((enableFpFusion.has_value() && enableFpFusion.value()) &&
               !fastMath.has_value()) {

[anmyachev]

FYI: finally it passes: https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15572699540/job/43851688164. Most likely the reason is #4479

[anmyachev]

Instead of reverting the SPV extension, can we do the change below?

diff --git a/third_party/intel/triton_xpu.cc b/third_party/intel/triton_xpu.cc
index 908d57c2e6..286eadd7f5 100644
--- a/third_party/intel/triton_xpu.cc
+++ b/third_party/intel/triton_xpu.cc
@@ -296,7 +296,7 @@ void init_triton_intel(py::module &&m) {
         if (auto *op = dyn_cast<FPMathOperator>(&inst)) {
           FastMathFlags FMF;
           // Default to allow contract when default fp fusion is not disabled.
-          if ((!enableFpFusion.has_value() || enableFpFusion.value()) &&
+          if ((enableFpFusion.has_value() && enableFpFusion.value()) &&
               !fastMath.has_value()) {

I'll try locally first.

UPD it works as well. However we may not have to make this change, since without using freezing option one of the models passes #4479 (comment). Let's look at the rest

[anmyachev]

Instead of reverting the SPV extension, can we do the change below?

diff --git a/third_party/intel/triton_xpu.cc b/third_party/intel/triton_xpu.cc
index 908d57c2e6..286eadd7f5 100644
--- a/third_party/intel/triton_xpu.cc
+++ b/third_party/intel/triton_xpu.cc
@@ -296,7 +296,7 @@ void init_triton_intel(py::module &&m) {
         if (auto *op = dyn_cast<FPMathOperator>(&inst)) {
           FastMathFlags FMF;
           // Default to allow contract when default fp fusion is not disabled.
-          if ((!enableFpFusion.has_value() || enableFpFusion.value()) &&
+          if ((enableFpFusion.has_value() && enableFpFusion.value()) &&
               !fastMath.has_value()) {

@whitneywhtsang according to results from Inductor tests: https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15630616934/job/44033895910 it's not an option.

[alexbaden]

What is the impact of this change on our micro benchmarks?

[whitneywhtsang]

@whitneywhtsang according to results from Inductor tests: https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15630616934/job/44033895910 it's not an option.

The reversal of 353d6ff and the suggested change should behave the same unless TRITON_DEFAULT_FP_FUSION is set explicitly.
Do those test cases only fail with the suggested change?

[anmyachev]

What is the impact of this change on our micro benchmarks?

I don't know

Do those test cases only fail with the suggested change?

It looks like we have a regression regardless of these changes, between https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15632063117 and https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15607709679

[anmyachev]

What is the impact of this change on our micro benchmarks?

Should we run benchmarks to get information?

[anmyachev]

Benchmarks run: https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15641089963. I'm probably done for today. If you can see that there are no regressions, we can merge it today. Since there is no more regression in Inductor tests.

[whitneywhtsang]

Started another one https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/15641240442 with special tag pr-4473, so it will be easier to use Grafana to check for performance impact later.

[whitneywhtsang]

Please create an issue to track reverting this change.
The change LGTM assuming no performance degradation.

[anmyachev]

Please create an issue to track reverting this change. The change LGTM assuming no performance degradation.

#4514