search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
26.79k stars 10.98k forks source link

[AArch64] Add SimplifyDemandedVectorEltsForTargetNode support for AArch64ISD::DUPLANE nodes #87497

Open RKSimon opened 3 months ago

RKSimon commented 3 months ago

Noticed on #86284 - aarch64 is missing SimplifyDemandedVectorElts handling to/from AArch64ISD::DUPLANE nodes

llvmbot commented 3 months ago

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

llvmbot commented 3 months ago

@llvm/issue-subscribers-good-first-issue

Author: Simon Pilgrim (RKSimon)

Noticed on #86284 - aarch64 is missing SimplifyDemandedVectorElts handling to/from AArch64ISD::DUPLANE nodes - [ ] Extend performDUPCombine to call SimplifyDemandedVectorElts from AArch64ISD::DUPLANE nodes as we only demand that one lane element. - [ ] Add AArch64TargetLowering::SimplifyDemandedVectorEltsForTargetNode to handle AArch64ISD::DUPLANE nodes (and AArch64ISD::DUP?) - if we only demand the dup lane index we might be able to remove the node entirely. - [ ] The hadd-combine.ll tests might be enough, but we could require some additional test coverage.
llvmbot commented 3 months ago

@llvm/issue-subscribers-backend-aarch64

Author: Simon Pilgrim (RKSimon)

Noticed on #86284 - aarch64 is missing SimplifyDemandedVectorElts handling to/from AArch64ISD::DUPLANE nodes - [ ] Extend performDUPCombine to call SimplifyDemandedVectorElts from AArch64ISD::DUPLANE nodes as we only demand that one lane element. - [ ] Add AArch64TargetLowering::SimplifyDemandedVectorEltsForTargetNode to handle AArch64ISD::DUPLANE nodes (and AArch64ISD::DUP?) - if we only demand the dup lane index we might be able to remove the node entirely. - [ ] The hadd-combine.ll tests might be enough, but we could require some additional test coverage.
aniplcc commented 3 months ago

Soo, just to clarify what is expected (in more granular terms)

  1. Add SimplifyDemandedVectorEltsForTargetNode() for AArch64 in: Target/AArch64/AArch64ISelLowering.cpp
    and make a case for handling AArch64ISD::DUP && AArch64ISD::DUPLANE nodes
  2. In performDUPCombine, add a case to match AArch64ISD::DUPLANE, then use the call to the newly implemented SimplifyDemandedVectorEltsForTargetNode. Is this right?
RKSimon commented 3 months ago

Soo, just to clarify what is expected (in more granular terms)

1. Add SimplifyDemandedVectorEltsForTargetNode() for AArch64 in: Target/AArch64/AArch64ISelLowering.cpp
   and make a case for handling AArch64ISD::DUP && AArch64ISD::DUPLANE nodes

Actually, I think we just need to handle AArch64ISD::DUPLANE - if the incoming DemandedElts mask is JUST the lane index bit then we can return the DUPLANE source directly (I think - not sure is the source can be a different vector width?). Otherwise we need to call a new DemandedElts mask on the source vector which is just the lane index.

2. In performDUPCombine, add a case to match AArch64ISD::DUPLANE, then use the call to the newly implemented SimplifyDemandedVectorEltsForTargetNode.
   Is this right?

We never call SimplifyDemandedVectorEltsForTargetNode directly - it will be a call to TLI.SimplifyDemandedVectorElts

SahilPatidar commented 2 months ago

@RKSimon, I'd be happy to take this on if no one else is currently working on it.

RKSimon commented 2 months ago

@SahilPatidar I'd recommend first completing the issues that are already assigned (unless you already have a patch?)

SQDNK commented 2 months ago

Hi @RKSimon, can I take this issue?

SQDNK commented 2 months ago

Thanks - I'm currently working on this. Apologies for the delay.