[PATCH] D123198: [LibCalls] Add argument extension attributes to more functions.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/BuildLibCalls.h:38
+  FunctionCallee getOrInsertLibFunc(Module *M, const TargetLibraryInfo &TLI,
+                                LibFunc TheLibFunc,  StringRef Name,
+                                FunctionType *T, AttributeList AttributeList);
----------------
jonpa wrote:
> efriedma wrote:
> > Does getOrInsertLibFunc really need a "Name" argument?  We have TargetLibraryInfo; we can just look up the correct name.
> I removed it and it seems that it is nearly identical, but for one test case:
> 
> double-float-shrink-1.ll
> 
> ```
> -; MS64-NEXT:     [[LOGBF:%.*]] = call fast float @logbf(float [[F:%.*]])
> +; MS64-NEXT:     [[LOGBF:%.*]] = call fast float @_logbf(float [[F:%.*]])
> ```
> 
> On this target the name gets a different name with 'TLI.setAvailableWithName(LibFunc_logbf, "_logbf");'.
> 
> I am not sure what the difference here means, but I am hoping that it would work to make that call to the preferred version directly, so I kept this change and updated the test.
> 
> 
That's what we want this code to be doing, yes.

(Looking at the latest Microsoft documentation, I'm not sure the TargetLibraryInfo description is actually doing what we want, but that's not really related to this patch.)


================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:1250
+      if (auto *IntTy = dyn_cast<IntegerType>(T->getParamType(i)))
+        assert(IntTy->getBitWidth() > 32 &&
+               "Integer parameter extension not handled.");
----------------
Do you need to check the bitwidth here?  There should be very few libfuncs which take a 64-bit integer as an argument, and I'd like to avoid tripping someone up if they, for example, add a call to a libfunc with a size_t argument and forget to test it on a 32-bit target.


================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:1238
+  // Make sure that any narrow integer argument is extended if required by
+  // the target ABI.
+  if (TLI.getExtAttrForI32Param() != Attribute::None)
----------------
jonpa wrote:
> efriedma wrote:
> > I'd prefer to make the assertion target-independent if possible; if someone is adding a new libcall optimization, they might not remember to write a test specifically for SystemZ.
> > 
> > Can we just add a check in the "default" case that the call doesn't pass/return any integer values, or something like that?  (I guess in that case, you'd have to list out all the calls we build that have a size_t argument or return value, but that should be a much smaller list than every function supported by inferNonMandatoryLibFuncAttrs.)
> Ah, that seems to work: if we can assume that size_t never needs extension (which I "hope") we get only a small list of functions here to exclude and can have the check enabled for all targets...
> 
I think it's true at the moment that size_t never needs extension.  If some future target does need extension, we have a list of functions we'd need to mark up.  (I can imagine some 64-bit target with an alternate 32-bit ABI.)


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

https://reviews.llvm.org/D123198