[llvm-commits] [patch] ARM NEON VREV patterns

[Bob Wilson]

Anton,

Thanks for working on this.  I've committed your patch with some  
changes.  For future reference, here are some comments:

* The llvm-gcc tests that I sent you as samples are pretty bad tests  
-- all the code is dead so if you optimize them they get reduced to  
empty functions.  It looked like your test was derived from those  
samples because it had the same problem.  I changed it to match the  
other Neon types, especially in loading the input vectors from  
pointers passed in to the test functions and returning the results.   
For the sake of completeness, I also added some tests with floating- 
point vectors.  Thanks for converting your test to use FileCheck --  
now I know how to do it!  I hope to convert all the Neon tests to use  
FileCheck soon.

* In the PatFrags that you added, it is better to hardcode the "undef"  
shuffle argument.  There's no point in specifying undef as an argument  
every time you use one of these.

* I added some new classes in the .td file to factor out the common  
parts of the VREV instructions.  It makes the code a lot easier to  
read and modify.

* I added comments before the isVREVMask function.

* As mentioned in the LLVM coding standards, you should try to match  
the indentation in the files you modify.  The isVREVMask function had  
everything indented by 4 spaces, even though the rest of that file  
(and most of LLVM) uses 2-space indentation.

Thanks again for doing this.  I hope you will be able to implement  
other VEXT soon!

On Jul 25, 2009, at 1:10 AM, Anton A. Korzh wrote:

> Changed to FileCheck method, thanks for advice.
>
> В Птн, 24/07/2009 в 14:38 -0700, Bill Wendling пишет:
>> On Fri, Jul 24, 2009 at 4:29 AM, Anton A. Korzh<anton at korzh.ru>  
>> wrote:
>>> Index: test/CodeGen/ARM/vrev.ll
>>> ===================================================================
>>> --- test/CodeGen/ARM/vrev.ll    (revision 0)
>>> +++ test/CodeGen/ARM/vrev.ll    (revision 0)
>>> @@ -0,0 +1,175 @@
>>> +; RUN: llvm-as < %s | llc -march=arm -mattr=+neon >%t
>>> +; RUN: grep {vrev64\\.8} %t | count 2
>>> +; RUN: grep {vrev64\\.16} %t | count 2
>>> +; RUN: grep {vrev64\\.32} %t | count 2
>>> +; RUN: grep {vrev32\\.8} %t | count 2
>>> +; RUN: grep {vrev32\\.16} %t | count 2
>>> +; RUN: grep {vrev16\\.8} %t | count 2
>>> +
>> Could you use the new FileCheck method for checking these?
>>
>> (Caveat: I haven't reviewed the rest of the patch)
>>
>> -bw
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> <vrev.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits