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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
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.");
----------------
efriedma wrote:
> 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.
I tried checking for any integer argument here which succeeded on tests after adding some more functions.

We by this now also see two vararg functions here (snprintf, vsnprintf). They have a size_t argument which we consider ok, but they also have varargs, which could *theoritically* be emitted by the compiler as well, right? It seems that currently this is done only by SimplifyLibCalls which removes a check and simply passes on the varargs to the new function, so they really should be ok as they are, I think.

I can imagine we either:

1. Accept these varargs as "safe" and assume they will never ever be emitted by an optimizer on its own and so ignores adding any mandatory attributes on them. We could add a comment somewhere documenting this.

2. We check the varargs here with an assert that any i32 arg is extended if target demands it.

3. We rewrite the emission of these libcalls so that it is part of the code design that the valist is being reused (as emitted by the front end). (Maybe this could if desirable be done at some later point?)

#2 and #3 would include all of them and not just the two with a size_t arg...



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

https://reviews.llvm.org/D123198