[PATCH] D131731: [InstCombine] Remove assumptions about int having 32 bits

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope accepted this revision.
bjope added a comment.
This revision is now accepted and ready to land.

I still think it is hard to understand for which functions `isValidProtoForLibFunc` was modified in the earlier commit that both refactored and modified the behavior in the same commit. For an out-of-tree target maintainer I need to understand how this impacted our downstream modifications to `isValidProtoForLibFunc`, while at the same time understanding if the changes impact our target (which isn't just having 16-bit ints, but for example char is also 16-bits, etc). Anyway, now I guess the only way is forward (even if other out-of-tree target maintainers might end up having the same trouble when rebasing later), so I guess I'll stop dwelling about that.

LGTM, as this seem to solve problems seen with D129915 <https://reviews.llvm.org/D129915>.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2712
+  if (FormatStr.size() == 1 || FormatStr == "%%") {
+    // Convert the character to unsigned char before passing it to putchar
+    // to avoid host-specific sign extension in the IR.  Putchar converts
----------------
I do not fully understand the part with "Putchar converts it to unsigned char regardless". Is that referring to the libc implementation , or llvm::emitPutChar, or what?

Not sure if we can deal with this better regardless of the answer. I guess this would work for in-tree targets and "normal" compiler hosts. But `unsigned char` on the host may have a different size compared to size of `unsigned char` in the runtime libc implementation for the target. Since the `getConstantStringInfo` call above assume that a char is 8 bits (at least the in-tree version of `getConstantStringInfo`) and the compiler probably is built for a host with 8 bit char, then at least the compiler code match up here.

(FWIW, our OOT target with 16-bit char is now guarded by getConstantStringInfo returning false, while in the past I think isValidProtoForLibFunc might have guarded from even calling LibCallSimplifier::optimizePuts and LibCallSimplifier::optimizePrintFString.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131731