[ARM] Remove exit-on-error flag from test (PR27765)

[rovka]

Summary

The exit-on-error flag is necessary in order to avoid an unreachable in the DAGTypeLegalizer, when trying to expand a physical register. We can also avoid this situation by introducing a bitcast early on, where the invalid scalar-to-vector conversion is detected.

Fixes PR27765.

[rengolin]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

452

I take it that this transformation is harmless, since the compilation is going to fail anyway.

But I'm worried that this could trigger unrelated errors down the pipe until the last error is reported and the program exits, and confuse the user.

[rovka]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

452

Not taking this lightly, but we're not seeing any other errors now, and I'm not sure how you could recover in a way that would guarantee you won't see any bogus errors ever (while still reporting all the useful ones). I think it should be pretty safe to have a bitcast between two values that have the same number of bits (assertion just above, at line 445).

I'm open to other suggestions though :)

[rengolin]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

452

Right... maybe @bogner or @echristo can shed some light?

[rovka]

Updated the test to use something other than undef as input too. This way we can test 2 different code paths (the one for undef is a bit special because it really doesn't create a bitcast node, it just creates an undef with the type we're bitcasting to). With this change, we're testing something a bit more realistic, and the fix still seems to work.

I can't find any tests for this error on other targets. Should I try to add one at least for X86?

[rengolin]

In D21061#452291, @rovka wrote:

Updated the test to use something other than undef as input too. This way we can test 2 different code paths (the one for undef is a bit special because it really doesn't create a bitcast node, it just creates an undef with the type we're bitcasting to). With this change, we're testing something a bit more realistic, and the fix still seems to work.

Right, good idea. That gives me more confidence on the patch. :)

I can't find any tests for this error on other targets. Should I try to add one at least for X86?

If you can, that'd be great. It would ease my worries about other arches.

cheers,
--renato

[rovka]

In D21061#453339, @rengolin wrote:

In D21061#452291, @rovka wrote:

I can't find any tests for this error on other targets. Should I try to add one at least for X86?

If you can, that'd be great. It would ease my worries about other arches.

Ok, working on it.

However, I think your concern is more general - on any test where we check for one error, we might want to make sure we don't trigger other bogus errors down the line. Should all our tests that check for one error also make sure there aren't any unexpected errors issued? I can put that in my backlog if it sounds like something we want.

[rengolin]

In D21061#454522, @rovka wrote:

However, I think your concern is more general - on any test where we check for one error, we might want to make sure we don't trigger other bogus errors down the line. Should all our tests that check for one error also make sure there aren't any unexpected errors issued? I can put that in my backlog if it sounds like something we want.

I think it's impossible to predict everything. I'm happy with some other arch's test for now.

[rovka]

Added a test for PowerPC.

X86 proved a bit tricky, because there is some special code in X86TargetLowering::getRegForInlineAsmConstraint in case the generic getRegForInlineAsmConstraint didn't return what X86 wanted. It even has this comment:
Handle references to XMM physical registers that got mapped into the
wrong class. This can happen with constraints like {xmm0} where the
target independent register mapper will just pick the first match it can
find, ignoring the required type.

So I'm guessing X86 is trying pretty hard to avoid this code path, so even if I managed to make it go through there it would probably be a bug.

PowerPC on the other hand seems to be handling things more similarly to ARM, and it can also cope with the bitcast that this patch is adding.

[rengolin]

LGTM, thanks!

[rovka]

Ok, thanks, I'll leave this open for a few more days in case other people want to chime in (especially from other targets).

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

static void getCopyToParts(SelectionDAG &DAG, SDLoc DL,
   }
 
   // The value may have changed - recompute ValueVT.
   ValueVT = Val.getValueType();
   assert(NumParts * PartBits == ValueVT.getSizeInBits() &&
          "Failed to tile the value with PartVT!");
 
   if (NumParts == 1) {
-    if (PartEVT != ValueVT)
+    if (PartEVT != ValueVT) {
       diagnosePossiblyInvalidConstraint(*DAG.getContext(), V,
                                         "scalar-to-vector conversion failed");
+      Val = DAG.getNode(ISD::BITCAST, DL, PartVT, Val);
+    }
 
     Parts[0] = Val;
     return;
   }
 
   // Expand the value into multiple parts.
   if (NumParts & (NumParts - 1)) {
     // The number of parts is not a power of 2.  Split off and copy the tail.

test/CodeGen/ARM/2012-09-25-InlineAsmScalarToVectorConv2.ll

-; RUN: not llc -mtriple=arm-eabi -mcpu=cortex-a8 -exit-on-error %s -o - 2>&1 | FileCheck %s
+; RUN: not llc -mtriple=arm-eabi -mcpu=cortex-a8 %s -o - 2>&1 | FileCheck %s
 
 ; Check for error message:
 ; CHECK: scalar-to-vector conversion failed, possible invalid constraint for vector type
+; CHECK: scalar-to-vector conversion failed, possible invalid constraint for vector type
 
 define hidden void @f(i32* %corr, i32 %order) nounwind ssp {
-  tail call void asm sideeffect "vst1.s32 { ${1:q}, ${2:q} }, [$0]", "r,{q0},{q1}"(i32* %corr, <2 x i64>* undef, <2 x i64>* undef) nounwind, !srcloc !0
+  tail call void asm sideeffect "vst1.s32 { ${1:q}, ${2:q} }, [$0]", "r,{q0},{q1}"(i32* %corr, <2 x i64>* undef, i32 %order) nounwind, !srcloc !0
   ret void
 }
 
 !0 = !{i32 257}