This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][MC] Add support for experimental Zawrs extension
ClosedPublic

Authored by reames on Sep 7 2022, 12:02 PM.

Details

Summary

This implements the Zawrs extension as specified here: https://github.com/riscv/riscv-zawrs/releases/download/V1.0-rc3/Zawrs.pdf. Despite the 1.0 version name, this does not appear to have been ratified, so putting it under experimental for the moment. I have been told that the current version is near final, and unlikely to change (again), but have nothing to cite on that.

This change adds assembly support, but does not include C language or IR intrinsics. We can decide if we want them, and handle that in a separate patch.

There were two prior attempts at implementing this.

D128235 by @palmer-dabbelt implements a prior version of this extension. Very annoyingly, the specification appears to have changed *without* a change in version number. This patch also didn't make the extension experimental.

D129462 by @sunshaoce implements the current version, but was abandoned due to confusion with the prior. Additionally, it's missing a few tests. I took the .td file change and the valid assembly test from that change.

Diff Detail

Event Timeline

reames created this revision.Sep 7 2022, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 12:02 PM
reames requested review of this revision.Sep 7 2022, 12:02 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 7 2022, 12:02 PM
reames edited the summary of this revision. (Show Details)Sep 7 2022, 12:05 PM
jrtc27 added inline comments.Sep 7 2022, 12:09 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
709

This doesn't really belong here, but a separate RISCVInstrInfoZawrs.td also seems a little overkill... hmm

llvm/lib/Target/RISCV/RISCVSubtarget.h
95

I would say keep these sorted but this seems to be a bit of a mess...

reames added inline comments.Sep 7 2022, 12:11 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
709

I had the same thoughts. I'm happy to defer to reviewers' preference.

llvm/lib/Target/RISCV/RISCVSubtarget.h
95

I'm happy to sort in a separate change if you'd like. Preferably after this lands. :)

asb added a comment.Sep 14 2022, 12:41 PM

The change in set of instructions without changing the version number is concerning - do you know anyone involved in that group? It would be good to feedback the difficulties this can cause for us. It's also not clear if there might be changes again during ratification without changing the version number, which wouldn't be ideal (though probably just about livable given it's marked as experimental).

Let's discuss briefly in the meeting tomorrow. Implementation-wise it all seems straightforward.

reames added a comment.EditedSep 14 2022, 5:20 PM

The change in set of instructions without changing the version number is concerning - do you know anyone involved in that group? It would be good to feedback the difficulties this can cause for us. It's also not clear if there might be changes again during ratification without changing the version number, which wouldn't be ideal (though probably just about livable given it's marked as experimental).

I agree that the incompatible change is highly sub-optimal. I've asked around a bit on the status here, but nothing I would consider "official" or would want to quote someone on. My overall take is that we probably should take this as experimental, but only with the understanding that it may change in incompatible ways.

Edit: For clarity, the incompatible change was between two -rcN variants of the 1.0 spec. So maybe that's "less bad"? If we include the candidate number, we do have a unique version ID? And maybe we should not be citing release candidates? Anyways, something to discuss tomorrow.

Let's discuss briefly in the meeting tomorrow. Implementation-wise it all seems straightforward.

Definitely.

asb added a comment.Sep 15 2022, 9:08 AM

I think the summary of our discussion on this was:

  • The versioning confusion is unfortunate - ideally there would be discussion elsewhere at RVI on improving the situation (either ELF attributes to indicate extensions are experimental, or making that unnecessary via never using 1.0 until something is ratified)
  • But the above isn't a blocker to merging. As the extension is gated by the experimental flag, even if there are last minute changes the impact on users should be minimal / non-existent.

I think the summary of our discussion on this was:

  • The versioning confusion is unfortunate - ideally there would be discussion elsewhere at RVI on improving the situation (either ELF attributes to indicate extensions are experimental, or making that unnecessary via never using 1.0 until something is ratified)
  • But the above isn't a blocker to merging. As the extension is gated by the experimental flag, even if there are last minute changes the impact on users should be minimal / non-existent.

Matches my takeaway. I'm going to rebase this and add in a doc change which clearly notes the release candidate bit.

reames updated this revision to Diff 460447.Sep 15 2022, 9:42 AM

Add docs.

asb requested changes to this revision.Sep 16 2022, 8:22 AM

Everything that's in this patch looks good to me - it's just missing some simple round-trip tests in the style of rv32zicboz-valid.s (and perhaps an -invalid.s that shows a sensible error message being produced when the instructions have an argument).

In terms of which *.td file to edit - RISCVInstrInfoA.td is another possibility. The plan had been that for the z?foo extensions, the letter after the z indicated which of the top-level extensions it was most relevant to - e.g. the zc* extensions (ztso predated this idea). I don't mind much either way - I'm happy with it in either location.

This revision now requires changes to proceed.Sep 16 2022, 8:22 AM

Everything that's in this patch looks good to me - it's just missing some simple round-trip tests in the style of rv32zicboz-valid.s (and perhaps an -invalid.s that shows a sensible error message being produced when the instructions have an argument).

You had me very confused at first as I'd written the valid tests - until I realized they'd been lost in the rebase. Updated patch forthcoming.

reames updated this revision to Diff 461345.Sep 19 2022, 1:39 PM
asb added a comment.Sep 19 2022, 10:37 PM

It looks like they're still missing in this updated version of the patch?

Regarding the overkill of "RISCVInstrInfoZawrs.td", how about having a "RISCVInstrInfoExtra.td" (or "RISCVInstrInfoExt.td") as a grab bag for everything that doesn't merit its own .td file?

I think this is fine regarding the versioning issue. Seems to only be missing the actual instruction tests, otherwise LGTM.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 20 2022, 10:15 AM
This revision was automatically updated to reflect the committed changes.

It looks like they're still missing in this updated version of the patch?

I have no idea what's going wrong here. I had been very careful to make sure the patch contained the new test file, but you're right, the revision in phab didn't.

Since we now had two LGTMs, and the tests had been in the original patch upload, I went ahead and landed. The tests are in the committed patch; if you want any changes, let me know.