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

[David Blaikie]

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

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