[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:
> 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.
I found that llvm.x86.avx512.vfmadd.ps.512 in fma-fneg-combine.ll would be lowered to general fma but it losts FMF. Do you have any comment to fix it ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90901