- User Since
- Nov 24 2016, 5:21 AM (228 w, 6 d)
Tue, Apr 6
Thu, Apr 1
Wed, Mar 31
Tue, Mar 30
Mon, Mar 29
Just wanted to add that the patch summary no longer matches the intent of the patch.
I agree, InstructionCost::Invalid is solving a specific problem which is different to "Did not bother computing a real cost".
Fri, Mar 26
Thu, Mar 25
OK, I understand you point. splat_vector(extract_vector_elt(vec, idx)) looks ok for me, and why you prefer do it in in SVEIntrinsicOpts.cpp ? what about do this in performdagcombine with AArch64TBL node?
The reason i prefer to handle in performdagcombine is that what we want to match is AArch64tbl ( ... splat_vector(..., constant)) rather than sve.tbl + sve.dupx. Since shufflevector can also convert to splat_vector.
I'm not saying all the pieces will come for free but this feels like an intrinsic optimisation problem rather than an instruction selection one. What about extending SVEIntrinsicOpts.cpp to convert the pattern to a stock splat_vector(extract_vector_elt(vec, idx)) and then letting the code generator decide how best to lower the LLVM way of doing things. This'll mean we solve the problem once for ACLE and auto-vectorisation.
Mon, Mar 22
Fri, Mar 19
A couple of niggles but otherwise looks good to me.
Thu, Mar 18
Wed, Mar 17
Tue, Mar 16
Mainly focused on the SVE side of things, which looks good to me.
I'm still struggling with how best to represent STEPVECTOR's scale operand. The patch as it currently stands is probably good enough but I do wonder how long we'll get by without needing to implement PromoteIntOp_STEP_VECTOR. I also wonder if we can take a similar approach as done for INSERT_SUBVECTOR's index operand.
Mar 15 2021
Mar 14 2021
Mar 11 2021
Mar 10 2021
Sorry I didn't mention this as part of my previous review but there should really be a test for each isel pattern. These do exist for the imm variants, it is just they were added to spillfill-sve.ll instead of sve-ld1-addressing-mode-reg-imm.ll. Given the name of the existing test file and the quantity of the new tests perhaps it is worth creating sve-ld1-addressing-mode-reg-reg.ll?
Mar 9 2021
I'm still working through the patch but here's what I've got for now.
Mar 5 2021
Mar 2 2021
Feb 25 2021
Feb 24 2021
Not sure of the value of the "ILLEGAL" tests given they're actually testing we can type legalise ISD::XOR, which I'm presuming is already well tested. That said it's nothing I cannot live with.
Feb 22 2021
Feb 19 2021
Feb 17 2021
One observation is that for arm_neon.h __inline__ is used. So perhaps we can just do likewise and we'll also be consistent across the two ACLE headers?
Feb 16 2021
Given the extensive testing within sve-fp-combine.ll I do wonder why sve-fma-dagcombine.ll is required, but otherwise the patch looks good.
Feb 15 2021
Patch looks good to me. I'll leave it up to you whether you want to extend the patch to cover f16/f64 cases or defer until needed.
Feb 12 2021
One minor issue that can be fixed when merging.
Feb 5 2021
Feb 4 2021
Jan 27 2021
Jan 26 2021
<A x Elt> llvm.experimental.vector.extract.elements(<B x Elt> %invec, i32 index, i32 stride)
Jan 24 2021
For now I'll just cover the IR side of things as the ISD node discussion raises different points and there's nothing to say they need to match.
Jan 22 2021
Jan 20 2021
Jan 16 2021
Jan 15 2021
Jan 14 2021
A bit of a flyby review as I'm still on holidays but to my mind many of the restrictions being proposed for the new intrinsic seem purely down to the design decision of splitting the input vector across two operands. I understand this is how the underlying instructions work for SVE but that does not seem like a good enough reason to compromise the IR.
Jan 13 2021
Jan 7 2021
Thanks @david-arm . FYI the CHECK-DAGs within sve-fixed-length-int-arith.ll don't need to be DAGs but I'm going to restructure these tests anyway so there's no point changing them.
Please can you add entries for nxv2f16 as well? That way all the legal fp types are covered.
Jan 6 2021
I'm just keeping things ticking over. It doesn't have to be this patch, I just did wanted to know if it's something I can take off my TODO list. If you're support eager then FABS also requires fixed length support :)
Do you plan to add support for fixed length vectors?
Jan 5 2021
As before I still believe there should be a test to protect this fix as presumably you're doing it for a reason.
Dec 27 2020
Dec 22 2020
Dec 21 2020
Fixed incorrect variable naming within ctlz_v64i8, ctpop_v64i8, cttz_v64i8.
Fixed incorrect variable naming with bitreverse_v64i8 test.
Made test function names consistent.