Page MenuHomePhabricator

[RISCV][Clang] Add support for Zmmul extension
ClosedPublic

Authored by ksyx on May 28 2021, 6:18 AM.

Details

Summary

This patch implements recently ratified extension Zmmul, a subextension of M (Integer Multiplication and Division) consisting only multiplication part of it.

Diff Detail

Event Timeline

ksyx created this revision.May 28 2021, 6:18 AM
ksyx requested review of this revision.May 28 2021, 6:18 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 28 2021, 6:18 AM

Do we need to add this to RISCVTargetStreamer::emitTargetAttributes?

Do we need to add this to RISCVTargetStreamer::emitTargetAttributes?

Yes, that, but also RISCVAsmParser::parseDirectiveAttribute and ELFObjectFileBase::getRISCVFeatures, though it seems that last one has been a bit neglected.

clang/lib/Basic/Targets/RISCV.cpp
268–269

These are supposed to be (approximately) sorted in terms of their canonical ordering. I don't know where Zmmul is proposed to go relative to the other Zfoo's, but it belongs down somewhere in there, I guess alphabetical if we don't know any better, not up here with the single-letter extensions.

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

Ditto

152

Ditto

Do we need to add this to RISCVTargetStreamer::emitTargetAttributes?

Yes, that, but also RISCVAsmParser::parseDirectiveAttribute and ELFObjectFileBase::getRISCVFeatures, though it seems that last one has been a bit neglected.

(we really need to cut down on how many places parse/produce arch strings...)

ksyx updated this revision to Diff 348608.May 28 2021, 6:18 PM
ksyx marked an inline comment as done.
  • Move down zmmul lines to fit the ordering
  • Add Zmmul into parseDirectiveAttribute
ksyx added a comment.May 28 2021, 6:19 PM

I have took a look into ELFObjectFile.cpp but I am not sure what work I need to do there since it seems other Z* extensions are not being handled there.

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

The pattern here seems to be that Z* extensions is following its parent their parent extensions, should we follow it? (See B and V)

ksyx added inline comments.May 28 2021, 6:23 PM
llvm/lib/Target/RISCV/RISCVSubtarget.h
48

But for Zfh it is not following HasStdExtF 🤔

Jim added a comment.Jun 9 2021, 11:33 PM

It should add arch string tests such as in clang/test/Driver/riscv-arch.c.

ksyx updated this revision to Diff 351083.Jun 10 2021, 1:04 AM

add: zmmul arch string test

Hi, do you still continue working on this patch?

Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 12:20 AM
ksyx added a comment.Mar 4 2022, 3:56 AM

Hi, do you still continue working on this patch?

In fact, not pretty sure what else I need to improve on this.

Hi, do you still continue working on this patch?

In fact, not pretty sure what else I need to improve on this.

At minimum it probably needs to rebased because RISCVISAInfo.cpp has been added to cut down on some duplication.

ksyx updated this revision to Diff 413211.Mar 5 2022, 6:33 AM

Rebase and adapt to new ISAInfo module.

ksyx updated this revision to Diff 413212.Mar 5 2022, 6:40 AM

Fix patch apply failed

ksyx updated this revision to Diff 440017.Jun 25 2022, 6:49 PM
ksyx retitled this revision from [RISCV][Clang] Implement support for zmmul-experimental to [RISCV][Clang] Add support for Zmmul extension.
ksyx edited the summary of this revision. (Show Details)

The Zmmul extension has just ratified. Update patch to rebase to current main.

craig.topper added inline comments.Jun 25 2022, 8:10 PM
llvm/lib/Target/RISCV/RISCV.td
18

This will cause Zmmul to appear in the ELF attributes any time is M is enabled. Is that what we want?

llvm/test/CodeGen/RISCV/attributes.ll
86–88

Is this a backwards compatibility issue?

ksyx updated this revision to Diff 440075.Jun 26 2022, 10:07 AM
ksyx marked an inline comment as done.

Make Zmmul extension independent instead of implied by M extension for backward ELF attribute compatibility.

ksyx marked an inline comment as done.Jun 26 2022, 10:10 AM
craig.topper requested changes to this revision.Jul 11 2022, 10:08 AM

Need to update RISCVTargetLowering::decomposeMulByConstant.

This revision now requires changes to proceed.Jul 11 2022, 10:08 AM
ksyx updated this revision to Diff 445332.Sun, Jul 17, 10:43 AM

Rebase and update use of M extension existence check to include Zmmul whenever needed.

craig.topper accepted this revision.Sun, Jul 17, 10:47 AM
craig.topper added inline comments.
llvm/lib/Support/RISCVISAInfo.cpp
80

If it is ratified, is the version really 0.1?

This revision is now accepted and ready to land.Sun, Jul 17, 10:47 AM
asb added inline comments.Mon, Jul 18, 2:36 AM
llvm/lib/Support/RISCVISAInfo.cpp
80
ksyx updated this revision to Diff 445461.Mon, Jul 18, 4:54 AM
ksyx marked an inline comment as done.

Update zmmul version & rebase

This revision was landed with ongoing or failed builds.Mon, Jul 18, 5:28 PM
This revision was automatically updated to reflect the committed changes.