Page MenuHomePhabricator

[RISCV] Add attribute support for all supported extensions
ClosedPublic

Authored by simoncook on Jan 18 2021, 12:32 PM.

Details

Summary

This adds support for ".attribute arch" for all extensions that are
currently supported by the compiler.

Depends on D94403.

Diff Detail

Event Timeline

simoncook created this revision.Jan 18 2021, 12:32 PM
simoncook requested review of this revision.Jan 18 2021, 12:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2021, 12:32 PM
jrtc27 added inline comments.Jan 18 2021, 12:42 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2154

Hmm how come this one is Std but too the other Zfoo's aren't?

simoncook added inline comments.Jan 18 2021, 2:24 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2154

It seems that when Zvlsseg was added, its name was committed inconsistent with all the others. This change just uses the features as currently defined.

jrtc27 added inline comments.Jan 18 2021, 2:26 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2154

Yeah. Arguably the Std is a waste of time in all of them (both Z and single-letter), it's implied by the names. I guess someone sufficiently-motivated should clean it up one way or the other.

Do you consider to modify ELFObjectFileBase::getRISCVFeatures() in llvm/lib/Object/ELFObjectFile.cpp?

Do you consider to modify ELFObjectFileBase::getRISCVFeatures() in llvm/lib/Object/ELFObjectFile.cpp?

Thanks for pointing that bit out. I was comparing against your previous work to check I'd covered everything, but missed that bit. I'll add an update to that file in this patch shortly.

simoncook updated this revision to Diff 317481.Jan 19 2021, 1:12 AM

Add support to llvm/lib/Object/ELFObjectFile.cpp

Extend disassembly test to check above change works via llvm-objdump

I think maybe we could extract the arch parser from driver[1] to llvm/lib/Support, so that we could just maintain one parser for driver and assembler, and there is also other potential user for that, like the C front-end for the target attribute, e.g. __attribute__ ((target ("arch=rv64gcv"))), and the linker can re-use that to read/merge/write the attribute too.

[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L171

I think maybe we could extract the arch parser from driver[1] to llvm/lib/Support, so that we could just maintain one parser for driver and assembler, and there is also other potential user for that, like the C front-end for the target attribute, e.g. __attribute__ ((target ("arch=rv64gcv"))), and the linker can re-use that to read/merge/write the attribute too.

[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L171

I was thinking about this too and was going to discuss it on Thursday's call. At the very least, even if we don't move the parser, the number of places that extension version numbers are add and used throughout the tools seems to call for it being centralised somewhere.

kito-cheng added a comment.EditedJan 19 2021, 1:36 AM

I think maybe we could extract the arch parser from driver[1] to llvm/lib/Support, so that we could just maintain one parser for driver and assembler, and there is also other potential user for that, like the C front-end for the target attribute, e.g. __attribute__ ((target ("arch=rv64gcv"))), and the linker can re-use that to read/merge/write the attribute too.

[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L171

I was thinking about this too and was going to discuss it on Thursday's call. At the very least, even if we don't move the parser, the number of places that extension version numbers are add and used throughout the tools seems to call for it being centralised somewhere.

Let's discuss that on the call :) The march parser rule is become very complicated now, so I think centralized that definitely is good idea.

asb added inline comments.Jan 21 2021, 5:24 AM
llvm/test/CodeGen/RISCV/attributes.ll
49

I suppose given the way this works in LLVM it would add even more duplication to reverse the logic for enabling sub-extensions and specify only rv32i2p0_b0p92, but @kito-cheng, can you please confirm what GCC does in a case like this?

jrtc27 added inline comments.Jan 21 2021, 5:38 AM
llvm/test/CodeGen/RISCV/attributes.ll
49

A single (extension -> extension list) list for all the implications would work for both uses I think?

kito-cheng added inline comments.Jan 21 2021, 7:55 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2155

Those attribute seems not output in canonical order, IIRC ld.bfd will check that during link stage.

e.g. when we have zfh and zbb, it should be rv32i_zfh_zbb rather than rv32i_zbb_zbf since the f > b in canonical order.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
92

Same as above here.

llvm/test/CodeGen/RISCV/attributes.ll
49

GCC & binutils will always expand the extension if it consisted by several sub-extensions, so yes, it doing same as GCC here.

50

I expect it should be v0p9_zvamo0p9_zvlsseg0p9 since V implied zvamo and zvlsseg.

63

rv32i2p0_zvlsseg0p9 rather than rv32i2p0_zvlsseg0p9 here, it's kind of weird, but in theory zvlsseg is not implied v.

zvamo is same situation too.

84

Ditto.

86

I would suggest you add some combination of zb*, zv* and zfh to make sure it output in canonical order.

llvm/test/MC/RISCV/attribute-arch.s
79

No v0p9 here.

simoncook added inline comments.Jan 21 2021, 8:15 AM
llvm/test/CodeGen/RISCV/attributes.ll
63

Ok thanks for clarifying this. Looking at RISCV.td it looks like the implications is the wrong way around zvamo implies v but not the other way around. I'll switch this the other way round for the update on this.

Note from the last sync up call, it's ok to landing that once review is done without refactor, refactor could be done separately after LLVM 12 release, that's won't be a blocker issue for this patch.

  • Emit extensions in canonical order
  • Update V tests to match current compiler support
  • Add tests for canonical order/parsing multiletter with version numbers
  • Fix parsing issue found by updated tests
simoncook updated this revision to Diff 318870.Jan 24 2021, 2:29 PM

Update for bitmanip 0.93

This revision is now accepted and ready to land.Jan 24 2021, 7:11 PM
This revision was landed with ongoing or failed builds.Jan 25 2021, 12:59 AM
This revision was automatically updated to reflect the committed changes.