[PATCH]Clang and AArch64 backend patches to support sshll/ushll instructions

Hao Liu Hao.Liu at arm.com
Mon Aug 19 09:52:41 PDT 2013


Hi Tim,

>> As I can't change vshll_n_s8(It must be Macro for the reason of 
>> checking the constant range), I use an local variable "a" in calling 
>> Macro vshll_n_s8 to forbid this conflict as following:

>Ok, that sounds reasonable. But it shouldn't be called "a". The reason most
of these variables start with "__" is because the user is free to write
"#define a something_evil" before including arm_neon.h, but identifiers
beginning with double-underscore are reserved for the compiler.
OK, may be rename it as "__a1" or something else.

>>         #define vshll_high_n_s8(a, __b) __extension__ ({ \
>>           int8x16_t __a = (a); \
>>           (int16x8_t)vshll_n_s8(vget_high_s8(a), __b); })

>That's more worrying, you're evaluating "a" twice there. What if someone
wrote "vshll_high_n(some_func(), 2)" where some_func had side-effects?
But __a can't be used, so it seems the solution maybe  :
         #define vshll_high_n_s8(a, __b) __extension__ ({ \
           int8x16_t __a = (a); \
           int8x16_t __a1 = (__a); \
           (int16x8_t)vshll_n_s8(vget_high_s8(__a1), __b); })
Or 
         #define vshll_high_n_s8(a, __b) __extension__ ({ \
           int8x16_t __a1 = (a); \
           (int16x8_t)vshll_n_s8(vget_high_s8(__a1), __b); })

But all these renaming results seem quite different and strange from the
existing code. 

>> However, I don't know whether the hack way is reasonable. If not, 
>> maybe we can just keep the current code and don't change it.

>I'm not sure, I'm afraid. I think as it stands your patch is a slight
improvement, but that may change if it gets more complicated to deal with
those issues.

Since the current code works well, I prefer to keep the code.
I think I'd discuss with Jiangning, or think out a better way to solve this
issue.

Thanks.






More information about the llvm-commits mailing list