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

Jonas Paulsson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 15 07:12:26 PDT 2022


jonpa updated this revision to Diff 423089.
jonpa added a comment.
Herald added subscribers: ormris, dexonsmith.

Thanks for review so far everyone! Since all of you seemed to like the idea of separating these mandatory arguments from the other ones ("inferred"), and I also thought it made sense, I decided to give it a try per your suggestions.

> This might make sense... I guess we could introduce a separate routine to compute the "mandatory" attributes. Maybe make it a combined routine, actually; have it call getOrInsertFunction, then compute the mandatory attributes for that function. That would let us be more strict, also: we could assert() that we don't build new libcalls that don't have the code for computing mandatory attributes.

I called this new function getOrInsertLibFunc() which is basically mirrors/wraps the getOrInsertFunction() definitions, with added handling of mandatory argument attributes. I also changed the name of inferLibFuncAttributes() to inferNonMandatoryLibFuncAttrs() to make this point clear.

The idea is now that getOrInsertLibFunc() is called instead of getOrInsertFunction() anytime a libcall is being created, for instance in LoopIdiomRecognize.cpp. It would have been nice to assert in getOrInsertFunction() that a libcall is not being created "on the side", but that does not seem practical as it would require checking with *TLI.

I left inferNonMandatoryLibFuncAttrs() outside of getOrInsertLibFunc() as it is not always called, for instance in emitFPutC().

I started out with demanding all functions in getOrInsertLibFunc() to be "seen", and listed also those without any i32 argument explicitly (with no action in the 'default' case). It seemed however more reasonable to instead do the check afterwards so that e.g. a new libfunction without any i32 argument would not all of a sudden trigger an assert. This is not "perfect", but at least it can report if there is not any type of extension at all being done on such an argument when the target requires it.

There are many more functions (found in annotate.ll) that are not currently being emitted in calls by any optimizer (or at least not tested). I added a test for one of them (LibFunc_ldexpl) as it actually can be generated by SimplifyLibCalls, but I am not sure what to do with the others. They could either be added now without any test, or they could simply be left out in the hopes that they would be handled/discovered if they ever where to be added later (what if someone makes an optimization but only tests it on their target and then clang is built in release-mode on SystemZ..? It would probably be missed then, but on the other hand there could always be new libfunctions added that we don't know about now...). The functions I have omitted for now are:

  case LibFunc_setitimer:
   case LibFunc_read:
   case LibFunc_write:
   case LibFunc_fdopen:
   case LibFunc_fputc:
   case LibFunc_fputc_unlocked:
   case LibFunc_fstat:
   case LibFunc_fstatvfs:
   case LibFunc_getitimer:
   case LibFunc_ungetc:
   case LibFunc_putc:
   case LibFunc_putc_unlocked:
   case LibFunc_under_IO_putc:
   case LibFunc_pread:
   case LibFunc_pwrite:
   case LibFunc_htonl:
   case LibFunc_htons:
   case LibFunc_ntohl:
   case LibFunc_ntohs:
   case LibFunc_fstat64:
   case LibFunc_fstatvfs64:
   case LibFunc_abs:
   case LibFunc_ffs:
   case LibFunc_isascii:
   case LibFunc_isdigit:
   case LibFunc_toascii:
   case LibFunc_putchar_unlocked:
     Changed |= setArgExtAttr(F, 0, TLI);
  
   case LibFunc_strchr:
   case LibFunc_strrchr:
   case LibFunc_memchr:
   case LibFunc_memrchr:
   case LibFunc_access:
   case LibFunc_fgets:
   case LibFunc_fgets_unlocked:
   case LibFunc_open:
   case LibFunc_open64:
   case LibFunc_memset_pattern4:
   case LibFunc_memset_pattern8:
   case LibFunc_memset_pattern16:
   case LibFunc_memset:
   case LibFunc_memset_chk:
   case LibFunc_memrchr:
   case LibFunc_strrchr:
     Changed |= setArgExtAttr(F, 1, TLI);
  
   case LibFunc_strtol:
   case LibFunc_strtoll:
   case LibFunc_strtoul:
   case LibFunc_strtoull:
   case LibFunc_setvbuf:
   case LibFunc_memccpy:
   case LibFunc_fseek:
   case LibFunc_fseeko:
   case LibFunc_fseeko64:
     Changed |= setArgExtAttr(F, 2, TLI);

Should any of these be added at this point?

BuildLibCalls:
Changed getFloatFnName() to getFloatFn() and made it also return the LibFunc via reference. This is needed as it may happen that the original LibFunc is first mapped to a name that is specific to a target. Then it happened that this name was not mapped back to the LibFunc, so I had to pass the LibFunc argument (in addition to the name) to getOrInsertLibFunc(). This mapping from name to LibFunc however seems to work fine in emitUnaryFloatFnCall() and emitBinaryFloatFnCall() ...

Since TLI is now needed in getOrInsertLibFunc() I added it where it was previously missing (/optional) to versions of emitUnaryFloatFnCall() and emitBinaryFloatFnCall(), as well as in SimplifyLibCalls.cpp as needed.

Tests:
Removed the previous changes in annotate.ll and instead tested the functions handled especially on SystemZ in some files. In two cases I had to remove the datalayout string so that there would not be i32 arguments on SystemZ (for instance for a pointer or size_t), but I think those test changes should be ok.


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

https://reviews.llvm.org/D123198

Files:
  llvm/include/llvm/IR/Module.h
  llvm/include/llvm/Transforms/Utils/BuildLibCalls.h
  llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp
  llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
  llvm/lib/Transforms/Utils/BuildLibCalls.cpp
  llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
  llvm/test/Transforms/InferFunctionAttrs/annotate.ll
  llvm/test/Transforms/InstCombine/exp2-1.ll
  llvm/test/Transforms/InstCombine/fortify-folding.ll
  llvm/test/Transforms/InstCombine/fputs-1.ll
  llvm/test/Transforms/InstCombine/puts-1.ll
  llvm/test/Transforms/InstCombine/strchr-1.ll
  llvm/test/Transforms/InstCombine/strstr-1.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D123198.423089.patch
Type: text/x-patch
Size: 41793 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220415/7540e439/attachment-0001.bin>


More information about the llvm-commits mailing list