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

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
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
----------------
bjope wrote:
> 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.)
The `putchar` comment refers to the libc implementation of the function on the target.  The host `putchar` shouldn't come into play here, but I've seen the assumption that `char` is 8 bits wide on both the host as on the target hardwired into transformations that don't depend on `getConstantStringInfo`, such as `optimizeMemChr` and `optimizeStrChr` (I'd be surprised if those were the only ones).  If other sizes are meant to be supported these assumptions should be reviewed and if necessary removed.  I don't have access to such targets but I'm willing to help with that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131731