[PATCH] D90901: [X86] Don't fold (fneg (fma (fneg X), Y, (fneg Z))) to (fma X, Y, Z)

[Jim Lin via Phabricator via llvm-commits]

Jim added inline comments.


================
Comment at: llvm/test/CodeGen/X86/avx2-fma-fneg-combine.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-unknown-unknown -mattr=+avx2,+fma | FileCheck %s --check-prefix=X32
-; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx2,+fma | FileCheck %s --check-prefix=X64
+; RUN: llc < %s -mtriple=i686-unknown-unknown -mattr=+avx2,+fma --enable-no-signed-zeros-fp-math | FileCheck %s --check-prefix=X32
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx2,+fma --enable-no-signed-zeros-fp-math | FileCheck %s --check-prefix=X64
----------------
spatel wrote:
> To confirm: this patch is based on a local update with the test changes?
> 
> We really want to avoid adding llc options to toggle the FP state as discussed in D99080 (so this diff would invert what we want to do there).
> 
> Unfortunately, this is a yak shaving exercise because the tests are using deprecated target-specific intrinsics. I hopefully fixed that for this file at least here:
> 8854b27b19
> 
> Ideally, we can now add tests with FMF (nsz on the appropriate instructions) to preserve the intent of these tests and also demonstrate the bug.
> 
> The fma-fneg-combine.ll file has the same problem. Let me know if I should update that or if you want to give that a try.
I don't understand what do you mean "this patch is based on a local update with the test changes?"

I am not sure how to update the tests in fma-fneg-combine.ll. The intrinsics used in fma-fneg-combine.ll have additional arguments not just only 3 arguments. 

I would update avx2-fma-fneg-combine.ll by adding FMF on the intrunctions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90901/new/

https://reviews.llvm.org/D90901