This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add MC support of RISCV Zcb Extension
ClosedPublic

Authored by VincentWu on Aug 3 2022, 10:04 PM.

Details

Summary

This patch add the instructions of Zcb extension.

Instructions in zcb extensions shorten part of bit manipulation instructions.

Diff Detail

Event Timeline

VincentWu created this revision.Aug 3 2022, 10:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2022, 10:04 PM
VincentWu requested review of this revision.Aug 3 2022, 10:04 PM
MaskRay added inline comments.Aug 3 2022, 11:49 PM
llvm/test/MC/RISCV/rv32zcb-Invalid.s
1 ↗(On Diff #449868)

< %s => %s

craig.topper added inline comments.Aug 8 2022, 12:29 PM
llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
11

Is this the right version?

54

80 columns

68

80 columns

99

Please fill out the scheduling information to match the non-compressed forms.

103

Does these require Zbb as well or does Zcb imply Zbb?

109

Does this require M or Zmmul or does Zcb imply M/Zmmul?

llvm/test/MC/RISCV/rv32zcb-Invalid.s
1 ↗(On Diff #449868)

Don't capitalize "Invalid" in the test name.

llvm/test/MC/RISCV/rv64zcb-Invalid.s
1 ↗(On Diff #449868)

Don't capitalize Invalid in the test name

kito-cheng added inline comments.Aug 21 2022, 6:41 AM
llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
109
kito-cheng added inline comments.Aug 21 2022, 6:42 AM
llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
109

Oh you mean require m or zmmul here , just ignore my comments :P

VincentWu updated this revision to Diff 454502.Aug 22 2022, 7:46 AM
VincentWu marked 11 inline comments as done.

rebase & address comments

VincentWu updated this revision to Diff 455059.Aug 23 2022, 8:35 PM

correct Predicates

craig.topper added inline comments.Aug 27 2022, 3:52 PM
llvm/lib/Target/RISCV/RISCV.td
366 ↗(On Diff #455059)

Should this implication also be in RISCVISAInfo.cpp? Right now I think specifying Zcb in the -march will put zca in the ELF attributes, but won't define __riscv_zca.

llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
121

Use the Sched information for LBU.

122

Please indent the body of this.

129

Use the Sched information for LHU.

craig.topper added inline comments.Aug 27 2022, 3:52 PM
llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
132

Indent

139

Indent

162

Need InstAliases for load/store with no immediate specified. Similar to this alias from RISCVInstrInfoC.td

def : InstAlias<"c.lw $rd, (${rs1})", (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
19 ↗(On Diff #455059)

If we put the write Sched information to match the uncompressed forms, is this still needed?

llvm/test/MC/RISCV/rv32zcb-valid.s
14

Please check to end of line using {{$}}

llvm/test/MC/RISCV/rv64zcb-invalid.s
1 ↗(On Diff #455059)

Use a single test file for riscv32 and riscv64

llvm/test/MC/RISCV/rv64zcb-valid.s
2

Use a single test file for riscv32 and riscv64. Except for riscv64 only instructions.

Are you going to add the CompressPats in a different patch?

VincentWu updated this revision to Diff 456190.Aug 28 2022, 8:50 AM
VincentWu marked 10 inline comments as done.

address comments

Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2022, 8:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Are you going to add the CompressPats in a different patch?

yes, I plan submit it as CodeGen part.
But I can add it into this patch if it is required.

llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
19 ↗(On Diff #455059)

Sorry, could you explain what is "uncompressed forms"? and where it is?

craig.topper added inline comments.Aug 28 2022, 9:07 AM
llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
19 ↗(On Diff #455059)

Every compressed instruction in Zcb has an equivalent instruction with a 32-bit encoding. Both instructions should have the same scheduling information.

VincentWu updated this revision to Diff 456224.Aug 28 2022, 6:50 PM
VincentWu marked 2 inline comments as done.

set zcb inst Sched info same as their uncompressed forms

VincentWu updated this revision to Diff 456235.Aug 28 2022, 8:04 PM

Add CompressPats for zcb

jrtc27 added inline comments.Aug 28 2022, 8:10 PM
llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
170

This feels wrong to me, but decompression is a bit dodgy if you can have all of Zcb without some of the extensions that its instructions decompress to?

178

Space after // (repeated multiple times)

196

These comments are pointless (repeated multiple times)

216

These don't line up, either pad them properly or don't bother at all

240

These aren't indented anywhere in llvm/lib/Target/RISCV

241

Why do you have at least two spaces after the comma here for every line? If you want to make them line up then there should be at least one where there's only one space.

VincentWu updated this revision to Diff 456542.Aug 29 2022, 9:12 PM
VincentWu marked 6 inline comments as done.

address comments

craig.topper added inline comments.Sep 21 2022, 6:03 PM
llvm/lib/Target/RISCV/RISCV.td
368 ↗(On Diff #456542)

The A should be lined up with the " on the line above.

llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
171

The second CompressPat needs let isCompressOnly = true in. See the CompressPats for C_ADD.

223

I think C_LW the wrong instruction?

223

Do you have tests for these patterns?

llvm/test/MC/RISCV/rv32zcb-valid.s
22

Needs {{$}} at the end of the line.

97

ANDI -> andi

145

ADDI -> addi

But what is this testing?

149

What is this testing?

llvm/test/MC/RISCV/rv64zcb-valid.s
17

Need {{$}} at the end of the line.

VincentWu updated this revision to Diff 463187.Sep 27 2022, 4:45 AM
VincentWu marked 9 inline comments as done.

address comments

craig.topper edited the summary of this revision. (Show Details)

Rebase

Add back the new files

Add CSZN format as named in the spec.
Reuse CLoad_ri/CStore_rri classes instead of creating new ones that were almost the same.
Use CS_ALU class for c.mul.

Update for rename CS_ALU to CA_ALU

craig.topper updated this revision to Diff 492310.EditedJan 25 2023, 7:03 PM

Merge uimm32_zc into uimm2.
Add RISCVOp::OPERAND_UIMM2_LSB0

Fix typo in RISCVBaseInfo.h

Fix RUN line that was duplicate testing riscv64

VincentWu updated this revision to Diff 492318.Jan 25 2023, 8:19 PM

rebase to upstream

VincentWu updated this revision to Diff 492319.Jan 25 2023, 8:32 PM

reset HEAD back to Diff 492315.

@VincentWu are you ok with the changes I made?

Update RISCVUsage.rst

@VincentWu are you ok with the changes I made?

sure, thank you for your work )

should I update zcb to v1.0 in this patch?
or create a new one

should I update zcb to v1.0 in this patch?
or create a new one

Let's go ahead and commit this as is. I'll add Zcb to my version bump patch D142596

This revision is now accepted and ready to land.Jan 26 2023, 9:19 AM

Possibly we want to add this change to the release notes?

llvm/lib/Target/RISCV/RISCVFeatures.td
304–308

Is there any particular reason why this is described as a "shortened format" instead of the more common "compressed"?

llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
10–11

Ditto "Code-size reduction" -> "compressed"?

luismarques accepted this revision.Jan 26 2023, 10:09 AM

LGTM, with some in-line caveats.

llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
2

Zc -> Zc*

llvm/test/MC/RISCV/rv32zcb-valid.s
22

Still has the old message phrasing with the "shortened".

Update file header

This revision was landed with ongoing or failed builds.Jan 26 2023, 12:54 PM
This revision was automatically updated to reflect the committed changes.
luke957 removed a subscriber: luke957.Jan 26 2023, 9:38 PM