[PATCH] D49273: [InstCombine] Expand the simplification of pow() into exp2()

[Mikael Holmén via Phabricator via llvm-commits]

uabelho added inline comments.


================
Comment at: llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp:1276
+      else
+        return emitUnaryFloatFnCall(FMul, TLI->getName(LibFunc_exp2), B, Attrs);
+    }
----------------
uabelho wrote:
> efriedma wrote:
> > uabelho wrote:
> > > On my target, exp2f is available, but exp2 isn't, so the
> > > 
> > >  hasUnaryFloatFn(TLI, Ty, LibFunc_exp2, LibFunc_exp2f, LibFunc_exp2l)
> > > 
> > > guard above returns true, but then when we try to find the name of LibFunc_exp2 we get "".
> > > 
> > > emitUnaryFloatFnCall doesn't check that the input name is something nice, it just happily adds an "f" to the name, and we end up with code trying to call the function "f" which eventually leads to a linking error.
> > > 
> > > Any thoughts about such cases?
> > I think the issue you're describing affects every user of emitUnaryFloatFnCall/emitBinaryFloatFnCall.
> > 
> > emitUnaryFloatFnCall should be fixed so it takes the same list of LibFunc enums that hasUnaryFloatFn does, and gets the appropriate name using getName().  The "appendTypeSuffix" helper is clearly inconsistent with the way TargetLibraryInfo is supposed to work.  I doubt the issue you're describing affects any in-tree target, but I'd approve the patch anyway as a cleanup.
> There are a few uses of emitUnaryFloatFnCall that I don't think are problematic atm, e.g. in optimizeDoubleFP. There we don't fetch the name via TLI, but pass in the name of the already existing function (which I suppose is the "double" version), and then I guess it works.
> 
> Anyway, it sounds good that you agree this is a problem and that it should work even if the "float" function is available and the "double" isn't like on my target.
> 
> I can try to make a cleanup patch that improves this in some way.
> 
> Thanks!
I created something here:
 https://reviews.llvm.org/D53370
Let's continue the discussion there.


Repository:
  rL LLVM

https://reviews.llvm.org/D49273