[PATCH] D99438: [SimplifyLibCalls] Take size of int into consideration when emitting ldexp/ldexpf

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1461
             FTy.getReturnType() == FTy.getParamType(0) &&
-            FTy.getParamType(1)->isIntegerTy(32));
+            FTy.getParamType(1)->isIntegerTy(getIntSize()));
 
----------------
atrosinenko wrote:
> I wonder whether every support library implementation on every 16-bit target satisfies this. Another question is about other usages of `->isIntegerTy(32)` in this file. Meanwhile, at least one such usage, for `htonl`/`ntohl`, seems to be exactly according to specification. Maybe this should be postponed to other patches (provided the issues can be detected easily enough).
> 
> //Just as a note: there are 21 occurrences of `inIntegerTy(32)`, 17 ones of `isIntegerTy()` (of any size) and even 1 `isIntegerTy(16)` (for `htons`/`ntohs`).//
Yes, there are indeed more places that might need to be fixed. I figured it was easier to start off with only adding the framework for detecting "size of int" and used ldexp as a first use case.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1810
   if (AllowApprox && (isa<SIToFPInst>(Expo) || isa<UIToFPInst>(Expo))) {
-    if (Value *ExpoI = getIntToFPVal(Expo, B))
+    if (Value *ExpoI = getIntToFPVal(Expo, B, 32))
       return createPowWithIntegerExponent(Base, ExpoI, M, B);
----------------
atrosinenko wrote:
> Shouldn't it be `TLI->getIntSize()` as well?
This patch only fixes "ldexp" family of libcalls. I'm dealing with "powi" in D99439 (having this patch as parent).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99438