This is an archive of the discontinued LLVM Phabricator instance.

[llvm][SVE] Addressing mode for FF/NF loads.
ClosedPublic

Authored by fpetrogalli on Mar 15 2020, 9:20 PM.

Details

Summary

This patch adds addressing mode computation for the following SVE
instructions:

  • ldff1{s}<T1> { <Zt>.<T2> }, <Pg>/Z, [<Xn|SP>{, <Xm>{, lsl #imm}}]
  • ldnf1{s}<T1> { <Zt>.<T2> }, <Pg>/Z, [<Xn|SP>{, #<imm>, mul vl}]

Diff Detail

Event Timeline

fpetrogalli created this revision.Mar 15 2020, 9:20 PM
Herald added a reviewer: efriedma. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
andwar added inline comments.Mar 16 2020, 9:54 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
4488

Is this change needed here?

4498

Why do you have to update this method? I've tried to find the relation with other changes in this file and I failed :)

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1550

[Nit] - unrelated

llvm/test/CodeGen/AArch64/sve-intrinsics-loads-ff.ll
379

FIXME

fpetrogalli marked 4 inline comments as done.

Remove unused parameter from test case, and remove empty line.

Thank you for the review @andwar !

Francesco

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
4488

I wouldn't say needed, but:

  1. as far as I know, all types handled by this component in LLVM are legal: which meas that it is very unlikely that we will ever enter the brnach code following. If we ever do enter this branch, it means that something elase is broken before in the pipeline. Addressing mode decomposition doesn't seem to me the right place to catch it.
if (NumElts != 2 && NumElts != 4 && NumElts != 8 && NumElts != 16)
  return EVT();
  1. What this method does is "give me a scalable vector of N packed lanes of integers". Whether the "N" comes of a vector of i1 lanes, it doesn't matter. Hence, I think it is better to use ElementCount in input, not a vector EVT of i1lanes. So, I think we can also remove the first if condition.

All in all, I think it makes sense to make the changes you see in this method.

4498

The method SelectAddrModeIndexedSVE operates on the "memory VT" of the underlying SDNode. One it determines it, it uses it to computed the decomposition of the ADD into a Base and an integer MUL VL Offset.

I had to modify this method because it needs to learn how to extract the MemVT from the SDNodes that represent the LDNF/LDFF. I then rearranged the code for the PRF intrinsic because (to me) it made mode sense to handle all cases under the same switch.

It would be nice if we could use the same method to retrieve the memory VT for all the SVE LD intrinsic, but for the custom ISD nodes there is not such thing as getMemoryVT(), like there is for the MemSDNode class.

We could argue to use MemIntrinsicSDNode (which have the getMemoryVT method), but at this point in the lowering sequence the AArch64ISD nodes have replaced the input Intrinsic::aarch64_sve_ld... nodes, so I cannot cast the nodes into instances of MemIntrinsicSDNode.

andwar added inline comments.Mar 17 2020, 6:46 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
4488

Thank you for the explanation, it makes sense!

  • Still, these changes are unrelated to what this patch is for. I think that this sort of refactoring should happen in a separate patch. If you want to keep it here, could you please list the NFC changes in the commit message?
  • I think that with these changes this method can be replaced with getSVEContainerType.
4498

OK, now I understand, thanks! I think that it would be very helpful to explain this in a comment somewhere within the method. In particular, why does this method differentiate between custom ISD nodes and intrinsics? Otherwise the code is a bit counterintuitive.

It's unfortunate that such a small method requires nested switch statements.

4517–4518

This llvm_unreachable is a bit confusing here (relates to the nested switch). Wouldn't it make more sense to have a default case within the nested switch block?

llvm/test/CodeGen/AArch64/sve-intrinsics-loads-nf.ll
18

CHECK-NEXT?

Thank you for your review @andwar.

I have restored the original code. I will address merging getPackedVectorTypeFromPredicateType into getSVEContainerType in a separate patch, marked as NFC.

Francesco

fpetrogalli marked 5 inline comments as done.Mar 17 2020, 8:01 AM

I have added CHECK-NEXT where needed.

Francesco

Cosmetic changes to the formatting of the codegen pattern defs. NFC

Cheers for working on this, LGTM!

andwar accepted this revision.Mar 18 2020, 5:28 AM

LGTM

This revision is now accepted and ready to land.Mar 18 2020, 5:28 AM
This revision was automatically updated to reflect the committed changes.