Skip to content

Conversation

@kunalspathak
Copy link
Contributor

@kunalspathak kunalspathak commented Nov 8, 2023

This is similar to #93067, except that most of the code added in this PR was generated by the tool, the same tool that generated encodings in #94285. The only thing that I realized the tool does not generate is the entries in emitInsMayWriteToGCReg(). I could add it, but I think it is easier to do it manually.

I have also added method placeholders for predicate registes which will need to be fixed once we add predicate register support in LSRA. I think the next step for this PR would be to make sure the encodings are correctly generated by comparing it with cordistool or lldb. That won't be possible until we add the actual Z and predicate registers.

Another TODO would be to fix the PerfScore numbers, not sure where to check that information though.

Contributes to #93095

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 8, 2023
@ghost ghost assigned kunalspathak Nov 8, 2023
@ghost
Copy link

ghost commented Nov 8, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

This is similar to #93067, except that most of the code added in this PR was generated by the tool, the same tool that generated encodings in #94285. The only thing that I realized the tool does not generate is the entries in emitInsMayWriteToGCReg(). I could add it, but I think it is easier to do it manually.

I have also added method placeholders for predicate registes which will need to be fixed once we add predicate register support in LSRA. I think the next step for this PR would be to make sure the encodings are correctly generated by comparing it with cordistool or lldb. That won't be possible until we add the actual Z and predicate registers.

Contributes to #93095

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak kunalspathak changed the title Uzp1 Arm64/SVE: Add Uzp1 Nov 8, 2023
@kunalspathak
Copy link
Contributor Author


/*****************************************************************************
*
* Returns an encoding for the specified register used in the 'Vd' position
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Returns an encoding for the specified register used in the 'Vd' position
* Returns an encoding for the specified register used in the 'Pd' position


/*****************************************************************************
*
* Returns an encoding for the specified register used in the 'Vn' position
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Returns an encoding for the specified register used in the 'Vn' position
* Returns an encoding for the specified register used in the 'Pn' position


/*****************************************************************************
*
* Returns an encoding for the specified register used in the 'Vm' position
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Returns an encoding for the specified register used in the 'Vm' position
* Returns an encoding for the specified register used in the 'Pm' position

@a74nh
Copy link
Contributor

a74nh commented Nov 9, 2023

I have a WIP patch which adds one SVE API entry. I'll rebase it on top on this and switch it to use UZP1.

@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Nov 9, 2023
theEmitter->emitIns_R_R_R(INS_rorv, EA_4BYTE, REG_R8, REG_R9, REG_R10);

// TODO-SVE: Fix once we add predicate registers
theEmitter->emitIns_R_R_R(INS_uzp1, EA_8BYTE, REG_R0, REG_R1, REG_R2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is emitting a neon uzp1, it should be INS_sve_uzp1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. i will address this and fix the tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed and update the file in #94549

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the above change, you're going to have to add case INS_sve_uzp1 into emitIns_R_R_R()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes and many such cases in emitIns* methods.

@kunalspathak
Copy link
Contributor Author

Replaced by #95016 and #94765

@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI arm-sve Work related to arm64 SVE/SVE2 support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants