[PATCH] Implement aarch64 neon instruction class SIMD copy - LLVM
James Molloy
james.molloy at arm.com
Tue Oct 8 06:05:33 PDT 2013
Hi Kevin,
Overall looks good, but I have quite a few comments on individual areas.
Cheers,
James
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:75
@@ -74,2 +74,3 @@
addRegisterClass(MVT::v2i64, &AArch64::FPR128RegClass);
+ addRegisterClass(MVT::v8f16, &AArch64::FPR128RegClass);
addRegisterClass(MVT::v4f32, &AArch64::FPR128RegClass);
----------------
This patch doesn't implement half-float support, so this line seems out of place.
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:309
@@ -304,1 +308,3 @@
+ setOperationAction(ISD::VECTOR_SHUFFLE, MVT::v2i64, Custom);
+ setOperationAction(ISD::VECTOR_SHUFFLE, MVT::v8f16, Custom);
setOperationAction(ISD::VECTOR_SHUFFLE, MVT::v2f32, Custom);
----------------
Again, as the patch avoids full half-float support, this line seems out of place.
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:322
@@ -312,1 +321,3 @@
+ setOperationAction(ISD::CONCAT_VECTORS, MVT::v2i64, Legal);
+ setOperationAction(ISD::CONCAT_VECTORS, MVT::v8f16, Legal);
setOperationAction(ISD::CONCAT_VECTORS, MVT::v4f32, Legal);
----------------
Half-float, out of place.
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3505
@@ +3504,3 @@
+
+ unsigned NumElts = VT.getVectorNumElements();
+ bool isOnlyLowElement = true;
----------------
I believe this code is copied from the ARM (32-bit) backend. It's quite substantial, and it would be nice if it could be factored away somewhere in a helper function and reused by both backends.
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3640
@@ -3517,2 +3639,3 @@
if (Lane == -1) Lane = 0;
-
+ // Test if V1 is a BUILD_VECTOR
+ if (Lane == 0 && V1.getOpcode() == ISD::BUILD_VECTOR &&
----------------
This comment doesn't help understand the code below. The code actually checks if this is a BUILD_VECTOR with a non-constant operand in index 0, and the splat lane is 0.
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3642
@@ +3641,3 @@
+ if (Lane == 0 && V1.getOpcode() == ISD::BUILD_VECTOR &&
+ !isa<ConstantSDNode>(V1.getOperand(0))) {
+ bool IsScalarToVector = true;
----------------
This condition seems wrong to me. The code *looks* like it produces a VDUP of a constant to all lanes (as opposed to the VDUPLANE below).
Why does this only work when the operand is *not* a constant? I would have thought that the condition should be negated - && isa<ConstantSDNode>(V1.getOperand(0));
Also, why does it only work when the splat lane is 0? can it not be: isa<ConstantSDNode>(V1.getOperand(Lane))?
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3664
@@ +3663,3 @@
+ // output.
+ //Collect elements need to be inserted and their index.
+ SmallVector<int, 8> NV1Elt;
----------------
Pedantic: Space between // and C
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3686
@@ +3685,3 @@
+ bool IsV2Inserted = true;
+ if (Length - NV1Elt.size() < 1)
+ IsV1Inserted = false;
----------------
This is a weird-looking condition - the canonical condition would be "NV1Elt.size() == Length", would it not?
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3691
@@ +3690,3 @@
+
+ //Decide which to be inserted.
+ SDValue InsV = V1;
----------------
Pedantic: // space D.
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3695
@@ +3694,3 @@
+ SmallVector<int, 8> InsIndex = N1Index;
+ if (IsV1Inserted || IsV2Inserted) {
+ if (NV1Elt.size() > NV2Elt.size()) {
----------------
As these variables are only used once, I suggest removing them and just using .size() in the if condition:
if (NV1Elt.size() != Length || NV2Elt.size() != Length)
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3702
@@ +3701,3 @@
+ }
+ else
+ InsV = DAG.getNode(ISD::UNDEF, dl, VT);
----------------
Coding style: if you're using braces around the if(), then you should use braces around the else().
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3707
@@ +3706,3 @@
+ int V1EltNum = V1.getValueType().getVectorNumElements();
+ for (int InsertNum = 0, Index = (NV1Elt.size() > NV2Elt.size()) ?
+ NV2Elt.size() : NV1Elt.size();
----------------
Several changes here:
* I'd use "I" instead of "InsertNum" (nitpick, feel free to ignore)
* I'd use "E" instead of "Index" (or at least something that is less generic than Index - Index doesn't feel like the name for an "end" variable)
* Index = InsArray.size()
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3713
@@ +3712,3 @@
+ ExtV = V2;
+ InsArray[InsertNum] -= V1EltNum;
+ }
----------------
Instead of mutating InsArray in-place, I'd create a variable and change that. Something like:
int Mask = InsArray[InsertNum];
if (Mask > V1EltNum) {
ExtV = V2;
Mask -= V1EltNum);
}
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3698
@@ +3697,3 @@
+ InsV = V2;
+ InsArray = NV2Elt;
+ InsIndex = N2Index;
----------------
Would it be better to call this "InsMasks" or something instead of "InsArray"? That way we know that one array contains the masks, the other contains the indices.
================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3715
@@ +3714,3 @@
+ }
+ EVT EltVT = MVT::i32;
+ if(EltSize == 64)
----------------
Please add a comment as to why this is necessary! Won't this get legalized anyway?
================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:2488
@@ -2488,3 +2487,3 @@
(v8i8 (trunc (v8i16 (srl (v8i16 node:$Rn),
- (v8i16 (Neon_dupImm 8))))))>;
+ (v8i16 (Neon_vdup (i32 8)))))))>;
def _4s : PatFrag<(ops node:$Rn),
----------------
Can you say why these (i32 X) are needed?
================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:3350
@@ -3350,2 +3349,3 @@
def : Pat<(v8i16 (bitconvert (v16i8 VPR128:$src))), (v8i16 VPR128:$src)>;
+def : Pat<(v8f16 (bitconvert (v16i8 VPR128:$src))), (v8f16 VPR128:$src)>;
----------------
I thought this patch wasn't meant to add half-float support?
================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:4512
@@ -4456,7 +4511,3 @@
-defm SMOVxb_pattern : Neon_SMOVx_pattern<v16i8, v8i8, i8, neon_uimm4_bare,
- neon_uimm3_bare, SMOVxb>;
-defm SMOVxh_pattern : Neon_SMOVx_pattern<v8i16, v4i16, i16, neon_uimm3_bare,
- neon_uimm2_bare, SMOVxh>;
-defm SMOVxs_pattern : Neon_SMOVx_pattern<v4i32, v2i32, i32, neon_uimm2_bare,
- neon_uimm1_bare, SMOVxs>;
+defm : Neon_SMOVx_pattern<v16i8, v8i8, i8, neon_uimm4_bare,
+ neon_uimm3_bare, SMOVxb>;
----------------
These pattern match SMOV nodes - would it not be better to pattern match (sext (COPY ) ) instead? That way the copy coalescer could be more effective. Same for UMOV nodes (zext (COPY ) )
http://llvm-reviews.chandlerc.com/D1854
More information about the cfe-commits
mailing list