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

[Hao Liu]

Hello Tim and Ana,

I've made changes according to your review comments:
(1)  Use IR (sext/zext, shufflevector and shl) to implement the ACLE functions. As I also use shift left by immediate instruction, I also add support for the shl instruction.
As shufflevector will be translated to extract_subvector, I match extract_subvector in the .td file.
(2)  Use uimm3,uimm4,uimm5 instead of redundant code. But as uimm6 is Operand<i64>, I can't use to check i32. So I add an i32 operand which use the existing uimm6_asmoperand to check i32:
        +def imm0_63 : Operand<i32>, ImmLeaf<i32, [{ return Imm >= 0 && Imm < 63; }]> {
        +  let ParserMatchClass = uimm6_asmoperand;
        +}
(3) Reformat the code style of lines over 80 characters
(4) Replace -march=aarch64  with -mtriple=aarch64-none-linux-gnu and replace -triple=aarch64 with -triple aarch64-none-linux-gnu in test cases.

(5) For clang side:
As Ana said, 'H' is used to skip adding 'q' in arm_neon.td. Currently there are only Q can specify 128bit vector type, but all function names will be added a 'q' after function name.
For example:
      If I define in arm_neon.td:
            Def SHLL_N : SInst<"vshll_high_n", ,"Qc">;
      The corresponding function in arm_neon.h will be
           vshllq_high_n_s8(int8x16_t )
      I can't generate a function named " vshll_high_n_s8".
Actually, the "_high" type ACLE functions are newly added in ARM v8 ACLE. There are many other functions also name like this, such as vshrn_high and vmovn_high. So I add a reusable naming type 'H'.

About typestr.startswith("Q")  in NeonEmitter.cpp:
I checked that UQ" is illegal, and "Q" is always the first letter of each 128bit type. Also as typestr may contain multiple types, such as "iUiQiQUi", I can't use typestr.find("Q") to check whether it needs to add a 'q' after the name. And it is also very common to check a character in a string.
As I haven't find a better way to implement this, I don't modify this code yet.

Thanks,
-Hao


From: Ana Pazos [mailto:apazos at codeaurora.org]
Sent: Monday, August 12, 2013 9:09 PM
To: Hao Liu; tnorthover at apple.com; llvm-commits at cs.uiuc.edu
Cc: Jiangning Liu
Subject: RE: [PATCH]Clang and AArch64 backend patches to support sshll/ushll instructions

Hello Hao,

-       The tests need a full 'aarch64-linux-gnu' triple to avoid breaking on unsupported platforms.
So in the tests replace -march=aarch64  with -mtriple=aarch64-none-linux-gnu and replace -triple=aarch64 with -triple aarch64-none-linux-gnu

-       The use of H as a new size modifier
It seems what you need is just a condition to skip adding 'q' to the intrinsics name even when the type is classified as Q, correct?
Something like adding a record field "isShiftPart2" or some more generic field so it can be reused by other intrinsics that have similar naming requirement.
It will require a bit more clang changes, but it will be clearer what you are trying to do.
What do you think?

Thanks,
Ana.

-----Original Message-----
From: Tim Northover [mailto:t.p.northover at gmail.com]
Sent: Friday, August 09, 2013 8:16 PM
To: Hao Liu
Cc: Tim Northover; llvm-commits; Jiangning Liu
Subject: Re: [PATCH]Clang and AArch64 backend patches to support sshll/ushll instructions

Hi Hao,

Thanks for working on this, it's good to see more NEON progress.

I realise the 32-bit ARM target already has special intrinsics for these instructions, but I think they should be representable in normal assembly. For example, I think

int64x2_t do_shift(int32x2_t in) { return vshll_n_s32(in, 3); }

is basically:

define <2 x i64> @do_shift(<2 x i32> %in) {
    %tmp = sext <2 x i32> %in to <2 x i64>
    %shift = shl <2 x i64> %in, <2 x i64> <i64 3, i64 3>
    ret <2 x i64> %shift
}

The _high_ variants will probably involve a shufflevector to implemenet a concatenation.

Longer term, we know more native vector code is coming (OpenCL, auto-vectoriser, ...) so it makes even more sense than before to do this without convenient hacks.

Other than that, it mostly looks sensible. Some smallish issues, LLVM side:
+ In AsmParser, there's already isUImm<3>, isUImm<4> and isUImm<5>. No
need for extra code I think.
+ It looks like there are lines longer than 80 characters in
+ AArch64InstrNEON.td The spacing is generally dodgy there: we use 2
+ spaces There's some duplication going on: imm0_7 is surely pretty much
+ the
same thing as uimm3 from AArch64InstrInfo.td.

Clang side:
+ What's 'H' standing for in arm_neon.td? It's not necessarily a bad
choice, just curious.
+ In NeonEmitter.cpp: typestr.startswith("Q") looks dodgy. What
happens if someone writes "UQi" for example?

I've also not looked at AArch64ISelLowering.cpp in detail yet. Changes there are probably going to be radically different if we go for proper CodeGen instead of the intrinsic route.

Cheers.

Tim.


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2548782
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-AArch64Neon-shiftleft-shiftLong-movLong-v2.patch
Type: application/octet-stream
Size: 11692 bytes
Desc: clang-AArch64Neon-shiftleft-shiftLong-movLong-v2.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130814/4392d121/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-AArch64Neon-shiftleft-shiftLong-movLong-v2.patch
Type: application/octet-stream
Size: 28830 bytes
Desc: llvm-AArch64Neon-shiftleft-shiftLong-movLong-v2.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130814/4392d121/attachment-0001.obj>