Page MenuHomePhabricator

[RISCV] Enable the use of the old mucounteren name
AcceptedPublic

Authored by mmxsrup on Fri, Jul 31, 10:48 PM.

Details

Summary

RISC-V Privileged Specification 1.11 defines mcountinhibit, which has the same numeric CSR value as mucounteren from 1.09.1.
This patch enables the use of the old mucounteren name.

Diff Detail

Event Timeline

mmxsrup created this revision.Fri, Jul 31, 10:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jul 31, 10:48 PM
mmxsrup requested review of this revision.Fri, Jul 31, 10:48 PM
jrtc27 requested changes to this revision.Sat, Aug 1, 4:43 AM
jrtc27 added inline comments.
llvm/lib/Target/RISCV/RISCVSystemOperands.td
313–315

is the style used elsewhere in this file and the RISCVInstrInfo*.td files.

This revision now requires changes to proceed.Sat, Aug 1, 4:43 AM
jrtc27 added inline comments.Sat, Aug 1, 4:45 AM
llvm/lib/Target/RISCV/RISCVSystemOperands.td
313–315

and please also add a blank line to separate it from the mhpmeventX definitions, as is done with mcycle(h)/minstret(h)

mmxsrup updated this revision to Diff 282432.Sat, Aug 1, 11:30 PM
jrtc27 added a comment.Sun, Aug 2, 8:27 AM

Patch looks good but please fix the title and description of this revision. It's not about adding AltName, it's about adding mountinhibit itself, and as part of that you're including support for the older name too.

pzheng requested changes to this revision.Sun, Aug 2, 10:07 AM

The mcountinhibit CSR has already been added in d36f2c6a6c4b. Looks like the patch needs to be rebased?

This revision now requires changes to proceed.Sun, Aug 2, 10:07 AM
mmxsrup retitled this revision from [RISCV] Update AltName for mucounteren to [RISCV] Add mucounteren CSR.Sun, Aug 2, 10:22 AM
mmxsrup updated this revision to Diff 282466.Sun, Aug 2, 10:46 AM
mmxsrup marked 2 inline comments as done.

I rebased.

lenary accepted this revision.Mon, Aug 3, 1:52 AM

LGTM with one minor change. Once we get the LGTM from @jrtc27 and @pzheng, then I'll commit this for you @mmxsrup!

llvm/lib/Target/RISCV/RISCVSystemOperands.td
313
mmxsrup updated this revision to Diff 282542.Mon, Aug 3, 2:29 AM
pzheng added a comment.Mon, Aug 3, 8:18 AM

LGTM, thanks for rebasing the patch, @mmxsrup!

lenary added a comment.Tue, Aug 4, 1:34 AM

@pzheng please may you use the "Add Action…" dropdown to mark as approved?

pzheng accepted this revision.Tue, Aug 4, 7:49 AM

LGTM

lenary added a comment.Wed, Aug 5, 6:03 AM

@jrtc27 Do you have any remaining concerns?

jrtc27 accepted this revision.Wed, Aug 5, 6:06 AM

Looks fine, other than please mention something about it being an alias in the commit subject when landing as currently it reads as if it's a new CSR (and I'd also personally change "This patch enables the use of mucounteren." to "This patch enables the use of the old mucounteren name." to make it completely clear, but if the subject is fixed then I don't think it's _necessary_).

This revision is now accepted and ready to land.Wed, Aug 5, 6:06 AM
mmxsrup retitled this revision from [RISCV] Add mucounteren CSR to [RISCV] Enable the use of the old mucounteren name.Wed, Aug 5, 7:05 AM
mmxsrup edited the summary of this revision. (Show Details)