[PATCH] D73588: AMDGPU: Enhancement on FDIV lowering in AMDGPUCodeGenPrepare

Wed Feb 5 12:10:59 PST 2020

cfang marked 2 inline comments as done.
cfang added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:611
 //
-// 1/x -> rcp(x) when fast unsafe rcp is legal or fpmath >= 2.5ULP with
-//                                                denormals flushed.
+// 1/x -> rcp(x) when fdiv is allowed to be re-associated or rcp is accurate.
 //
----------------
arsenm wrote:
> cfang wrote:
> > arsenm wrote:
> > > This has nothing to do with reassociation
> > Division re-association:  a/b -> a * rcp(b), and one special case is 1.0/b => 1.0*rcp(b) = rcp(b).
> > This is how 1.0/x -> rcp(x) associated with "re-association".  
> This isn't reassocation. This is just special handling of 1.0/b. Nothing algebraic changes here. There's no multiply introduced here
Ok, it seems we have a different understanding here.

I think this is still just a naming issue. Originally the name is something like UnsafeMath? 
But I do think arcp also gives the permission to do 1.0/x -> rcp(x) even though no multitply is explicitly generated
(1.0 * rcp(x) = rcp(x)). 

Maybe we should go back to use the original name as HasFastUnsafeOptions to clear your confusion?

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:614
+// a/b -> a*rcp(b) when fdiv is allowed to be re-associated.
+static Value *lowerUsingRcp (Value *Num, Value *Den, bool CanReassociateFDiv,
+                             bool RcpIsAccurate, IRBuilder<> Builder,
----------------
arsenm wrote:
> cfang wrote:
> > arsenm wrote:
> > > This should not be referred to ass lowering
> > I am thinking of a different name.  Do you have a meaningful name for the function in mind?
> combineRcp?
Better to be somethingUseRcp and somethingUseFastFDiv. I am still not sure what something should be here.
optimizeFDivUsingRcp?

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

https://reviews.llvm.org/D73588