[PATCH] D90174: [HIP] Fix regressions due to fp contract change

[Artem Belevich via Phabricator via cfe-commits]

tra added a subscriber: scanon.
tra added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:486
+  if (LangOpts.HIP)
+    Options.AllowFPOpFusion = llvm::FPOpFusion::Standard;
+
----------------
yaxunl wrote:
> tra wrote:
> > yaxunl wrote:
> > > tra wrote:
> > > > I don't think it's a good idea to force this.
> > > > 
> > > > Perhaps a better way to address this would be to set HIP-specific default to Standard where CUDA does it:
> > > > https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/CompilerInvocation.cpp#L2415
> > > > 
> > > > Currently HIP inherits this setting from CUDA.
> > > We want to keep -ffp-contract=fast for frontend so that we can continue emitting fmul/fadd insts with contract flag in IR for HIP programs. We only want to change the backend fp fuse option. Currently there is no separate clang option to set backend fp fuse option.
> > I do not see any references to `AllowFPOpFusion` anywhere under `clang/` other than in this function.
> > Perhaps I'm missing something. How/where does it make a difference in the front-end other than setting the option for the back-end?
> > 
> > 
> > 
> -ffp-contract not only sets backend fp fuse option, but also setFPContractMode
> 
> https://github.com/llvm/llvm-project/blob/2e204e23911b1f8bd1463535da40c6e48747a138/clang/include/clang/Basic/LangOptions.h#L413
> 
> which enables allowFPContractAcrossStatement
> 
> https://github.com/llvm/llvm-project/blob/2e204e23911b1f8bd1463535da40c6e48747a138/clang/include/clang/Basic/LangOptions.h#L439
> 
> which sets llvm::FastMathFlags
> 
> https://github.com/llvm/llvm-project/blob/d3205bbca3e0002d76282878986993e7e7994779/clang/lib/CodeGen/CodeGenFunction.cpp#L129
> 
> which causes fmul/fadd to have contract flag
> 
> 
> 
> 
> 
> 
Thank you. I see.
I'm still uncomfortable with growing a target-specific quirk in FP behavior that can't be overridden from command line, but considering that it's limted to HIP only, it may be OK short-term. 

I think similar issue (fast-math does not allow changing contraction mode) was discussed last year:
http://lists.llvm.org/pipermail/llvm-dev/2019-July/133787.html

One thing I know about FP is that there are more nuances than I'm aware of. It may be worth asking with more experience with this.

@scanon -- do we have any better options to honor contraction pragmas with **implicitly**-enabled fast-math?



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

https://reviews.llvm.org/D90174