[llvm] r176066 - [fast-isel] Make sure the FastLowerArguments function checks to make sure the

[Chad Rosier]

On Feb 26, 2013, at 11:51 AM, David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Feb 26, 2013 at 11:48 AM, Chad Rosier <mcrosier at apple.com> wrote:
>> 
>> On Feb 25, 2013, at 6:33 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> 
>> 
>> 
>> On Mon, Feb 25, 2013 at 5:05 PM, Chad Rosier <mcrosier at apple.com> wrote:
>>> 
>>> Author: mcrosier
>>> Date: Mon Feb 25 19:05:31 2013
>>> New Revision: 176066
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=176066&view=rev
>>> Log:
>>> [fast-isel] Make sure the FastLowerArguments function checks to make sure
>>> the
>>> arguments type is a simple type.
>>> rdar://13290455
>> 
>> 
>> Test case?
>> 
>> 
>> David,
>> I'm not aware of an easy way to test this.
>> 
>> ; RUN: not llc < %s -fast-isel -fast-isel-abort -fast-isel-abort-args
>> -verify-machineinstrs -mtriple=x86_64-apple-darwin10
>> 
>> ; Abort because we don't know how to handle non-simple type i31.
>> define i31 @t1(i31 %a, i31 %b, i31 %c) {
>> entry:
>>  %add = add nsw i31 %b, %a
>>  %add1 = add nsw i31 %add, %c
>>  ret i31 %add1
>> }
>> 
>> The above test case will exercise the issue, but results in llc aborting and
>> the 'not' on the run line doesn't have the correct effect.
> 
> I'm by no means especially familiar with the backend, just looking at
> the change it seemed like something that I would expect to have an
> easily observable effect.

> I assume the -fast-isel-abort-args/-fast-isel-abort is what's meant to
> cause the abort if fast isel can't handle something?

Correct.

> Do we produce the
> same code before/after your change, but the only issue is that
> previously we would do it with fast isel & now we don't?

No, this was causing an assert when ArgVT.getSimpleVT() was called.

> What was the
> correctness problem (why make any change at all) then? If there was a
> correctness problem, can't we just test that in the actually assembly
> output rather than testing whether fast isel aborts?

The issue was a compiler assertion.

I could just remove the -fast-isel-abort-args flag to avoid causing the abort.  Would that suffice as a test?
I could comment that this previously caused an assert.

 Chad

> - David
> 
>> 
>> Chad
>> 
>> 
>>> 
>>> 
>>> Modified:
>>>    llvm/trunk/lib/Target/ARM/ARMFastISel.cpp
>>>    llvm/trunk/lib/Target/X86/X86FastISel.cpp
>>> 
>>> Modified: llvm/trunk/lib/Target/ARM/ARMFastISel.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMFastISel.cpp?rev=176066&r1=176065&r2=176066&view=diff
>>> 
>>> ==============================================================================
>>> --- llvm/trunk/lib/Target/ARM/ARMFastISel.cpp (original)
>>> +++ llvm/trunk/lib/Target/ARM/ARMFastISel.cpp Mon Feb 25 19:05:31 2013
>>> @@ -2922,6 +2922,7 @@ bool ARMFastISel::FastLowerArguments() {
>>>       return false;
>>> 
>>>     EVT ArgVT = TLI.getValueType(ArgTy);
>>> +    if (!ArgVT.isSimple()) return false;
>>>     switch (ArgVT.getSimpleVT().SimpleTy) {
>>>     case MVT::i8:
>>>     case MVT::i16:
>>> 
>>> Modified: llvm/trunk/lib/Target/X86/X86FastISel.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FastISel.cpp?rev=176066&r1=176065&r2=176066&view=diff
>>> 
>>> ==============================================================================
>>> --- llvm/trunk/lib/Target/X86/X86FastISel.cpp (original)
>>> +++ llvm/trunk/lib/Target/X86/X86FastISel.cpp Mon Feb 25 19:05:31 2013
>>> @@ -1555,6 +1555,7 @@ bool X86FastISel::FastLowerArguments() {
>>>       return false;
>>> 
>>>     EVT ArgVT = TLI.getValueType(ArgTy);
>>> +    if (!ArgVT.isSimple()) return false;
>>>     switch (ArgVT.getSimpleVT().SimpleTy) {
>>>     case MVT::i32:
>>>     case MVT::i64:
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130226/880a5c17/attachment.html>