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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7698
+      if (ResultType != Val.getValueType() &&
+          ResultType.getSizeInBits() == Val.getValueSizeInBits()) {
         Val = DAG.getNode(ISD::BITCAST, getCurSDLoc(),
----------------
thopre wrote:
> 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).
Oh, right, getCopyFromRegs doesn't do the bitcast because it thinks the destination type is i64.

I'm not really convinced going through i64 is the right approach here (it only does the right thing on ARM because i64 isn't legal), but I guess it's okay given the existing precedent.


================
Comment at: test/CodeGen/ARM/pr34170.ll:1
+; RUN: llc -mtriple armv7-arm-linux-gnueabihf -O2 -mcpu=cortex-a7 < %s | FileCheck %s
+
----------------
thopre wrote:
> 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?
Sometimes people use the PR number because there isn't any good alternative, but best practice is to describe what the file is testing, not why you're adding the test.  So something like inline-asm-float-gpr.ll.  (You can put a reference to the bug as a comment in the test if you think it's helpful.)


Repository:
  rL LLVM

https://reviews.llvm.org/D45376