This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add MC support of RISCV Zca Extension
ClosedPublic

Authored by VincentWu on Jul 19 2022, 8:54 PM.

Details

Summary

This patch adds support for part of Zc extension which will be frozen soon.

This extension is designed to continue reducing the binary size of RISC-V programs.
In this patch:
Zca is a subset of C extension instructions that are compatible with the Zc extension.

The spec of Zc ext is Here

Diff Detail

Event Timeline

VincentWu created this revision.Jul 19 2022, 8:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 8:54 PM
VincentWu requested review of this revision.Jul 19 2022, 8:54 PM

Initial shallow comments:

  • They're not ratified, they passed arch review 5 days ago and can be frozen. I don't know if the specs have even been updated to say frozen yet.
  • I see a bunch of style violations (missing indentation, missing spaces around tokens, newlines around curly braces where there shouldn't be, bad camel case, missing newlines at ends of files).
  • Please keep MC, codegen and optimisation pass patches separate, it makes it much easier to review.
  • Are these mnemonics, especially for push/pop's operands, accepted by the GNU community or just dreamt up by the authors of the spec?

Where is it documented that these extensions are ratified?

Putting 3 extensions in one patch is a guaranteed way to make it take longer to approved. The smaller you can make patches the quicker individual pieces can be reviewed and approved. I would also recommend putting new optimization passes in separate patches. MC layer and CodeGen should be separate patches.

Please run clang-format on patches.

llvm/lib/Object/ELFObjectFile.cpp
305 ↗(On Diff #446029)

Capitalize variable names

343 ↗(On Diff #446029)

Space after if. Space before curly brace

344 ↗(On Diff #446029)

Space before false

347 ↗(On Diff #446029)

Extra blank line

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2109 ↗(On Diff #446029)

Memonic -> Mnemonic

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
194 ↗(On Diff #446029)

Capitalize variable names

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
416 ↗(On Diff #446029)

Space after '<'

llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp
1 ↗(On Diff #446029)

Header banner should be exactly 80 characters long

20 ↗(On Diff #446029)

This says Zce but Zce is not mentioned in your patch description.

225 ↗(On Diff #446029)

Use the template version of getSubtarget that does this cast.

230 ↗(On Diff #446029)

getInstrInfo() on RISCVSubtarget* returns RISCVInstrInfo * this case shouldn't be needed.

llvm/lib/Target/RISCV/RISCVOptimizePushPop.cpp
1 ↗(On Diff #446029)

80 columns

23 ↗(On Diff #446029)

too many blank lines

27 ↗(On Diff #446029)

This is overly indented

44 ↗(On Diff #446029)

Capitalize variable names.

Can this a be a DenseMap? std::map is an ordered map which doesn't make much sense for pointers.

73 ↗(On Diff #446029)

Capitalize variable names

VincentWu edited the summary of this revision. (Show Details)Jul 20 2022, 1:39 AM
  • They're not ratified, they passed arch review 5 days ago and can be frozen. I don't know if the specs have even been updated to say frozen yet.

Apologies for my misrepresentation, I have corrected my description

  • I see a bunch of style violations (missing indentation, missing spaces around tokens, newlines around curly braces where there shouldn't be, bad camel case, missing newlines at ends of files).
  • Please keep MC, codegen and optimisation pass patches separate, it makes it much easier to review.

I will split the patch and update them soon.

  • Are these mnemonics, especially for push/pop's operands, accepted by the GNU community or just dreamt up by the authors of the spec?

Where is it documented that these extensions are ratified?

sorry for my wrong statements, I have corrected my description

Putting 3 extensions in one patch is a guaranteed way to make it take longer to approved. The smaller you can make patches the quicker individual pieces can be reviewed and approved. I would also recommend putting new optimization passes in separate patches. MC layer and CodeGen should be separate patches.

Please run clang-format on patches.

I will split this patch as much as passable soon.

VincentWu updated this revision to Diff 446135.Jul 20 2022, 6:19 AM
VincentWu marked 4 inline comments as done.
VincentWu retitled this revision from [RISCV] Add part support of RISCV Zc Extension (Zca, Zcb, Zcmp) to [RISCV] Add part support of RISCV Zc Extension (Zca).
VincentWu edited the summary of this revision. (Show Details)

Split this patch, which now contains only Zca's modifications.

format the patch

address comments

frasercrmck added inline comments.Jul 20 2022, 6:52 AM
llvm/lib/Object/ELFObjectFile.cpp
317 ↗(On Diff #446135)

This doesn't seem right to me. Why is zca treated this way but no other z* extensions are?

llvm/lib/Target/RISCV/RISCVFrameLowering.h
48 ↗(On Diff #446135)

Pushable with a capital P

EDIT: but this function isn't used?

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
937 ↗(On Diff #446135)

Should we have Subtarget.hasStdExtCOrStdExtZca?

In certain contexts, would even a Subtarget.hasCInstructions make sense, like we have Subtarget.hasVInstructions?

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
61 ↗(On Diff #446135)

Do we need to resort to checking feature bits in RISCVInstrInfo? Can't we use STI.hasStdExtC and friends?

1246 ↗(On Diff #446135)

I'd rather the fetching of the subtarget was taken out into a variable rather than repeating it twice.

Also can we cast this to a RISCVSubtarget and use hasStdExtC and friends instead of prodding feature bits?

This is still doing both MC and codegen. Please separate them as I said.

VincentWu updated this revision to Diff 447299.Jul 25 2022, 6:54 AM
VincentWu marked 4 inline comments as done.
VincentWu retitled this revision from [RISCV] Add part support of RISCV Zc Extension (Zca) to [RISCV] Add MC support of RISCV Zca Extension.

address some comments and split the patch into MC & CodeGen

craig.topper added inline comments.Jul 26 2022, 9:52 AM
llvm/lib/Target/RISCV/RISCV.td
332

16-bit in this is potentially confusing as it could also be read as the size of the data being loaded/stored. "compressed" might be more clear?

llvm/lib/Target/RISCV/RISCVInstrInfoC.td
408

Is adding Predicates here fixing a bug? Or was it getting the one on line 284 making this change redundant?

495

Doesn't C_LWSP need to be updated? It's sharing with C_FLDSP

555

C_SWSP needs to be updated

702

There's no predicate in effect for c.lw. This is a bug already in tree. I'll fix it.

craig.topper added inline comments.Jul 26 2022, 9:55 AM
llvm/lib/Target/RISCV/RISCVInstrInfoC.td
495

Ignore this I misread it.

555

Ignore this I misread it.

VincentWu updated this revision to Diff 448561.Jul 29 2022, 2:54 AM
VincentWu marked 6 inline comments as done.

address comments

VincentWu updated this revision to Diff 448570.Jul 29 2022, 3:21 AM

Fixed failed testcase

craig.topper added inline comments.Jul 29 2022, 3:14 PM
llvm/lib/Target/RISCV/RISCVInstrInfoC.td
698

Please rebase, I changed this section to fix c.lw, c.sw, c.lwsp, c.swsp to have predicates

VincentWu updated this revision to Diff 448796.Jul 30 2022, 2:56 AM
VincentWu marked 2 inline comments as done.

rebase

craig.topper added inline comments.Aug 8 2022, 12:19 PM
llvm/lib/Support/RISCVISAInfo.cpp
112

Can we alphabetize this?

llvm/test/MC/RISCV/rv32zca-invalid.s
2

Can this command line be added to rv32c-invalid.s? Test content appears to be the same

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

Can this be merged with rv32c-valid.s?

llvm/test/MC/RISCV/rv64zca-valid.s
20

Should these error mesages mention Zca?

jrtc27 added inline comments.Aug 8 2022, 12:28 PM
llvm/lib/Target/RISCV/RISCVSubtarget.h
170

Alphabetise

llvm/test/MC/RISCV/rv32c-only-valid.s
20

This needs or 'Zca' ... adding otherwise you're not checking what you think you are. Are there cases of this in other files too?

craig.topper added inline comments.Aug 8 2022, 1:41 PM
llvm/test/MC/RISCV/rv32c-only-valid.s
20

I've added {{$}} to the end of all of these lines in https://reviews.llvm.org/D131436 so we'll require all extensions to be listed.

VincentWu updated this revision to Diff 452868.Aug 15 2022, 6:24 PM
VincentWu marked 7 inline comments as done.

address comment
rebase
merge testcase for C and Zca

This revision is now accepted and ready to land.Aug 16 2022, 10:55 AM
This revision was automatically updated to reflect the committed changes.