[llvm-commits] [llvm] r127518 - in /llvm/trunk: lib/Target/ARM/ARMFastISel.cpp test/CodeGen/ARM/fast-isel-pred.ll

[Eric Christopher]

>> I agree this will fix the crash, but have to confess it kinda makes me a bit squirmy. If the instruction isn't predicable, it shouldn't have the operands. Is there no way we can tell the backend directly that some NEON instructions really aren't predicable? Note: I have not looked deeply into how predication+NEON is handled, so that could easily be anywhere from "trivial" to "holy crap hard" in complexity to do. I'm purely talking from the perspective of what I'd like to see at a high level. As you described it: the compiler shouldn't lie to itself.
> 

I agree, but the problem is in more than just this area and I haven't really looked into how to rewrite that part of the arm backend to fix it. Keep in mind that the normal isel also lies in this way - I'm guessing that the optional def is unconditionally added and then mucked with later during the selection dag timeframe.

>> Added: llvm/trunk/test/CodeGen/ARM/fast-isel-pred.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/fast-isel-pred.ll?rev=127518&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/ARM/fast-isel-pred.ll (added)
>> +++ llvm/trunk/test/CodeGen/ARM/fast-isel-pred.ll Fri Mar 11 19:09:29 2011
>> @@ -0,0 +1,60 @@
>> +; ModuleID = 'test.c'
>> +target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:32-f32:32:32-f64:32:32-v64:64:64-v128:128:128-a0:0:64-n32"
>> +target triple = "armv7-apple-darwin"
>> +
> On the completely nitpicky side, is the test really Darwin specific? Either way, the datalayout and triple lines aren't really necessary. Just passing -march/-mtriple and -mcpu to llc in the RUN line is more consistent with the rest of the tests.
> 

I make no claims about how well fast-isel works for darwin targets so it's turned off there - so this testcase isn't valid for non-darwin at the moment.

> This was code-gen OK before, but failed when trying to assemble due to an invalid instruction (predicate on a non-predicable instruction), right? If so, this test won't catch that as it doesn't try to assemble the assembly file. Adding a CHECK-NOT for the particular instruction that was bogus and a CHECK for what it actually should be would be good.

This would assert with a Debug+Asserts build.

> Not a huge deal, but seems a bit largish for this sort of test. Does it reduce down at all? I would, perhaps naively, expect that a conditional followed by the vcvt (sitofp <4 x i32> %1 to <4 x float>) would do the trick.

I believed the original submitter when she said she couldn't get it any smaller :)

-eric