[PATCH] D49517: [WebAssembly] Handle return type conversions in FixFunctionBitcasts pass

[Jacob Gravelle via Phabricator via llvm-commits]

jgravelle-google added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp:147
   else {
-    Wrapper->eraseFromParent();
-    return nullptr;
+    LLVM_DEBUG(dbgs() << "CreateWrapper failed due to return type mismatch: "
+                      << F->getName() << "\n");
----------------
Doesn't seem to fail in this case. Probably worth leaving the debug print though, just changing the wording to be more accurate.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp:150
+    Instruction::CastOps CastOpCode =
+        CastInst::getCastOpcode(Call, false, Ty->getReturnType(), false);
+    CastInst *Cast =
----------------
Should annotate the false values with the argument names, like `/*SrcIsSigned = */ false`


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp:156
 
   return Wrapper;
 }
----------------
Maybe debug-print something about how this branch successfully created a wrapper? We should probably print something along all the possible execution paths for this function.


================
Comment at: test/CodeGen/WebAssembly/function-bitcasts-varargs.ll:23
+; CHECK-NEXT: call    	.Lunderspecified_bitcast at FUNCTION, $pop1, $pop0
+; CHECK: call    	.Lspecified_bitcast at FUNCTION, $pop{{[0-9]+$}}
 
----------------
This is probably less fragile test-wise, as well as being easier to read later.


================
Comment at: test/CodeGen/WebAssembly/unsupported-function-bitcasts.ll:25
   call void bitcast (void (i64)* @has_i64_arg to void (i32)*)(i32 0)
-  %t = call i32 bitcast (i64 ()* @has_i64_ret to i32 ()*)()
   ret void
----------------
Should add this test elsewhere, along with other mismatched-return tests (float-to-int comes to mind). We do support return value conversions now, so we should have tests that exercise it.


Repository:
  rL LLVM

https://reviews.llvm.org/D49517