[PATCH] D104247: [DAGCombine] reassoc flag shouldn't enable contract

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13258
+    bool CanFuse = Options.UnsafeFPMath || N->getFlags().hasAllowContract();
     // fold (fsub (fma x, y, (fmul u, v)), z)
     //   -> (fma x, y (fma u, v, (fneg z)))
----------------
jsji wrote:
> jsji wrote:
> > spatel wrote:
> > > jsji wrote:
> > > > mcberg2017 wrote:
> > > > > Some of these patterns below have operands/operations which change order, they should be audited and checked reassoc FMF to enable.  Perhaps a follow on change?
> > > > Good point, sure, will follow up.
> > > Sorry, I missed this difference. The patch as-is is allowing miscompiles IIUC. For example, this test of the pattern:
> > > 
> > > ```
> > > ; x*y + u*v - z
> > > define float @f(float %x, float %y, float %z, float %u, float %v) {
> > >   %xy = fmul contract float %x, %y
> > >   %uv = fmul contract float %u, %v
> > >   %xyuv = fadd contract float %xy, %uv
> > >   %xyuvz = fsub contract float %xyuv, %z
> > >   ret float %xyuvz
> > > }
> > > 
> > > ```
> > > 
> > > We are not allowed to fuse the trailing fsub into fma based on what we decided in D89527, but that's what happens now with:
> > > 
> > > ```
> > > $ llc -o - fma.ll -mtriple=powerpc64le -ppc-asm-full-reg-names
> > > 	xsmsubasp f3, f4, f5 ; u*v - z
> > > 	xsmaddasp f3, f1, f2 ; x*y + (u*v - z)
> > > 	fmr f1, f3
> > > 	blr
> > > 
> > > ```
> > Yes, working on it.
> The specific case `; x*y + u*v - z` , the behavior is not changed by D104247. It has been like this for a long time. But yeah, I am looking into these patterns now.
https://reviews.llvm.org/D104723 posted for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104247