[PATCH] D38640: [WebAssembly] Narrow the scope of WebAssemblyFixFunctionBitcasts

[Jacob Gravelle via Phabricator via llvm-commits]

jgravelle-google added a comment.

In https://reviews.llvm.org/D38640#891197, @sunfish wrote:

> Is the assumption here that the eventual caller will cast the argument back to the original type?


Pretty much. In C, that is explicitly defined to be well-defined, and otherwise it's undefined behavior. Native targets ignore missing args and/or read junk bits from the argument registers, wasm traps at runtime, or with this pass we synthesize/ignore args.

> For example, in `test_argument`, if `call_func` bitcasts the `i32 ()*` argument to `void (i32)*` before calling it, then it doesn't want a wrapper. However, if it doesn't do that cast, and just calls it as `i32 ()*`, then it does seem to need the wrapper. So I think it's a guess either way. Do you think functions doing an extra cast are more common that functions that don't?

And if `call_func` stores it to memory, or prints it to screen, or compares it to the pointer value of `has_i32_arg`, we have no idea. Because it's a guess either way, and because we don't have a way to reverse the bitcast-function, I think the most reasonable thing to do is nothing.
Functions doing an extra cast are probably equally common (I have no data for this), but more importantly they're not-undefined, so I think we should make sure to support those, and any undefined use cases that we can support are a bonus.



================
Comment at: lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp:72
+      Instruction *I = dyn_cast<Instruction>(U.getUser());
+      if (!I || I->getOpcode() != Instruction::Call)
+        // Skip uses that aren't immediately called
----------------
dschuff wrote:
> How about invoke? Also, are we filtering out intrinsics anywhere? (An `IntrinsicInst` is a `CallInst` but I forget if its opcode is `Instruction::Call` or not)
Not sure, will check and add tests


https://reviews.llvm.org/D38640