[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:
> > 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 ?
Yes, it's a mess. Please have a look and update this patch after:
f66ba4cfa
333c968
f12f9be
9b59a61

If I got it right, we have the necessary test coverage in fma-fneg-combine.ll now, so there is no need to change the RUN lines in that file. Just regenerate the CHECK lines using utils/update_llc_test_checks.py after applying this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90901