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

[Jim Grosbach]

Re-cap summary of our offline convo:

On Mar 14, 2011, at 11:15 AM, Eric Christopher wrote:

> 
>>> 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.

Yah, I was just hoping to pique your curiosity to dig deeper. :) I agree it's a pre-existing issue and not a necessity to address as part of this bugfix.
 
> 
>>> 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.

Ah, right. Forgot it was turned off entirely for non-darwin. Just putting the info on the RUN line should take care of it, then.

> 
>> 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.

It'd still be good for the -Asserts build to be able to detect the failure, though, right? The CHECK/CHECK-NOT also help self-document what was going wrong.

>> 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 :)

As we talked about offline, we can probably get a bit smaller, but not too much. Fast-isel heuristics are probably playing some games here. IMO, worth trying a bit, but not spending a huge amount of time on it. Maybe just reduce to a single function and add an explanatory comment about what's really being checked for?

-Jim