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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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
----------------
Jim wrote:
> 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.
Sorry - I didn't realize the tests in fma-fneg-combine.ll were for still active target-specific nodes.
I posted D102725, so we can try to convert those to use IR-level FMF too.
I think you can update this patch to use IR-level FMF on avx2-fma-fneg-combine.ll now. It would be good to add tests with 'nsz' and leave the existing tests as-is. That way we'll show that we are not miscompiling but we are optimizing if possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90901