This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support .variant_cc directive for the assembler.
ClosedPublic

Authored by fakepaper56 on Nov 19 2022, 6:51 AM.

Details

Summary

The patch is split from D103435. The patch supported a new directive .variant_cc
that annotates function with STO_RISCV_VARIANT_CC. Symbols marked with
STO_RISCV_VARIANT_CC do not use standard calling conversion or use parameter not
passed in GPR/FPR.

Related: https://github.com/riscv/riscv-elf-psabi-doc/pull/190

Initial authored by: HsiangKai

Diff Detail

Event Timeline

fakepaper56 created this revision.Nov 19 2022, 6:51 AM
fakepaper56 requested review of this revision.Nov 19 2022, 6:51 AM

I think @HsiangKai is still active, so he may want to carry on the work? I agree that splitting the asm part from the feature is nice. I left some comments on D103435 about the MC style.

@MaskRay he (@HsiangKai) has hand-over all RISC-V related patch to other SiFive folks when he join Google :) Or maybe we should commandeer that revision and keep using that?

Address MaskRay's comment in D103435.

@MaskRay he (@HsiangKai) has hand-over all RISC-V related patch to other SiFive folks when he join Google :) Or maybe we should commandeer that revision and keep using that?

Thanks for the context! Using this patch is fine.

MaskRay added inline comments.Nov 21 2022, 2:48 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2314

in '.variant_cc' directive is not really useful as the diagnostic repeats the source line. ditto below

2318

This code self explains so the comment seems redundant. You may delete the blank line after Parser.Lex() as well.

llvm/test/MC/RISCV/directive-variant_cc-err.s
4 ↗(On Diff #476790)

Add line/column information like :[[#@LINE+1]]:25:. See some newer tests in llvm/test/MC/

Grep defsym=ERR=1. It's probably better to combine positive and negative tests in one file.

Refine error message and merge directive-variant_cc-err.s and directive-variant_cc.s

fakepaper56 marked 2 inline comments as done.Nov 21 2022, 9:18 PM
fakepaper56 added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2318

I use default expected newline for the token missing.

smd added a subscriber: smd.Nov 22 2022, 5:35 AM
fakepaper56 marked an inline comment as done.

Rebase and ping.

kito-cheng added inline comments.Nov 30 2022, 8:05 AM
llvm/test/MC/RISCV/directive-variant_cc.s
2

Add "readelf -s" test to address Kito's comment.

LGTM, @MaskRay do you mind have one more look for this? :)

MaskRay added inline comments.Dec 1 2022, 1:30 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2321

unneeded blank line

llvm/test/MC/RISCV/directive-variant_cc.s
2

We need local/global definitions as the aarch64 test does.

4

-filetype asm can be omitted.

Remove unneeded blank line and add local/global definitions into test.
I don't add filetype asm test since the patch does not include AsmPrinter
support. I will complete this part in the next patch.

MaskRay requested changes to this revision.Dec 2 2022, 10:08 AM
MaskRay added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2312

MCSymbol *Sym = getContext().lookupSymbol(SymbolName); is not the best. See D122507

This revision now requires changes to proceed.Dec 2 2022, 10:08 AM
MaskRay added inline comments.Dec 2 2022, 10:10 AM
llvm/test/MC/RISCV/directive-variant_cc.s
2

See directive-variant_pcs.s tests. I think local, def1, def2, alias_def1, undef are all useful.

We can do somewhat better by leveraging --defsym=ERR=1 (as you already did).

fakepaper56 added inline comments.Dec 2 2022, 7:55 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2312

Good point. Thank your reminder.

Allow unregistered symbol and add more tests.

fakepaper56 marked 4 inline comments as done.Dec 2 2022, 9:05 PM
MaskRay accepted this revision.Dec 4 2022, 10:45 AM
MaskRay added inline comments.
llvm/test/MC/RISCV/directive-variant_cc.s
40

Delete // ERR-NEXT: .variant_cc and this line. The error mark is tested by other tests and this without --strict-whitespace --match-full-lines isn't useful.

46

Delete this line. The error mark is tested by other tests and this without --strict-whitespace --match-full-lines isn't useful.

This revision is now accepted and ready to land.Dec 4 2022, 10:45 AM

Remove error mark tests.

fakepaper56 marked 2 inline comments as done.Dec 4 2022, 8:13 PM
This revision was landed with ongoing or failed builds.Dec 4 2022, 8:13 PM
This revision was automatically updated to reflect the committed changes.
smd added a comment.EditedDec 5 2022, 12:48 AM

Hi @fakepaper56

Do you think it's possible to support printing .variant_cc label to asm file if the proper flag for symbol is set? I believe this happens(didn't verify though) for .variant_pcs in AArch64ELFStreamer.cpp
This would help me with D132994

Thanks!

This comment was removed by fakepaper56.

I will upstream a patch for printing .variant_cc in asm soon. I think your requirement could be based on that.