[PATCH] D45376: Fix PR34170: Crash on inline asm with 64bit output in 32bit GPR

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added a comment.

Hi Eli,

Again my apologies for the delay, my mail filter put the notification for your comments in the wrong folder. Should be fixed now and I'll keep an eye on phabricator directly to make sure it is. I've replied to your comments inline, will add the float testcase that you asked shortly.



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7698
+      if (ResultType != Val.getValueType() &&
+          ResultType.getSizeInBits() == Val.getValueSizeInBits()) {
         Val = DAG.getNode(ISD::BITCAST, getCurSDLoc(),
----------------
efriedma wrote:
> Is this change necessary?  getCopyFromRegs should do the appropriate int->float bitcast, I think.
It's not about int->float but in this case about i32 (ResultType) -> i64 (Val). It's much in the same spirit as what happens in GetRegistersForValue for input operands. As you can see here we only record the type for output but don't do the bitcase since it must be done *after* the asm call (as opposed to inputs where it must be done before).


================
Comment at: test/CodeGen/ARM/pr34170.ll:1
+; RUN: llc -mtriple armv7-arm-linux-gnueabihf -O2 -mcpu=cortex-a7 < %s | FileCheck %s
+
----------------
efriedma wrote:
> Please pick a different filename for this file.  And I'd like to see tests for float and double inputs and outputs, in both soft-float and hard-float modes.
So as not to repeat the same mistake what is the rule on using the PR number for the test? I can see quite a few tests with the same sort of name, some with very close PR number. Would inline-asm-convert-crash.ll be ok?


Repository:
  rL LLVM

https://reviews.llvm.org/D45376