Page MenuHomePhabricator

[RISCV] add the MC layer support of riscv vector Zvamo extension
ClosedPublic

Authored by StephenFan on Jul 31 2020, 11:42 PM.

Diff Detail

Event Timeline

StephenFan requested review of this revision.Jul 31 2020, 11:42 PM
HsiangKai added inline comments.Aug 2 2020, 11:58 PM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
988

"If index EEW is greater than XLEN, an illegal instruction exception is raised."

We could collect all AMO instructions with EEW = 64 into one place and use "Predicates = [IsRV64]" to ensure XLEN >= EEW.

HsiangKai added inline comments.Aug 3 2020, 1:34 AM
llvm/lib/Target/RISCV/RISCV.td
152

Zvamo should imply 'A' extension and 'V' extension.

def FeatureExtZvamo
    : SubtargetFeature<"experimental-zvamo", "HasStdExtZvamo", "true",
                      "'Zvamo'(Vector AMO Operations)",
                      [FeatureStdExtA, FeatureStdExtV]>;
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
984

Zvamo should imply 'A' extension and 'V' extension. So, use HasStdExtZvamo is enough.

1029

Ditto.

llvm/test/MC/RISCV/rvv/zvamo.s
2

--mattr=+experimental-zvamo is enough.

16

instruction requires the following: 'Zvamo'(Vector AMO Operations)

remove the hasStdExtV and hasStdExtA predicates, and add the IsRV64 predicates

StephenFan marked an inline comment as done.
StephenFan marked an inline comment as done.

I confused the relationship of sub-extensions before. Sorry for that.
There are some discussions[0] about the relationship between vector sub-extensions. It seems that


V-extension implies Zvamo + Zvlsseg

Zvqmac, Zvamo and Zvlsseg are stand alone sub extensions. That is, Zvamo does not imply V extension.

Zvqmac has no implied extensions.
Zvamo implies A-extension.
Zvlsseg has no implied extensions.

Do you agree with that?

[0] https://github.com/riscv/riscv-v-spec/issues/546

Zvamo implies A-extension.

I think it's require instead of imply?

RVV Spec 8. Vector AMO Operations:

If vector AMO instructions are supported, then the scalar Zaamo instructions (atomic operations from the standard A extension) must be present.
StephenFan marked 3 inline comments as done.EditedAug 7 2020, 6:49 PM

I confused the relationship of sub-extensions before. Sorry for that.
There are some discussions[0] about the relationship between vector sub-extensions. It seems that


V-extension implies Zvamo + Zvlsseg

Zvqmac, Zvamo and Zvlsseg are stand alone sub extensions. That is, Zvamo does not imply V extension.

Zvqmac has no implied extensions.
Zvamo implies A-extension.
Zvlsseg has no implied extensions.

Do you agree with that?

[0] https://github.com/riscv/riscv-v-spec/issues/546

So, let me summarize:
Zvlsseg has no implied extensions
Zvqmac has no implied extensions
Zvamo required A extension ? If so, is it suitable that use -mattr=+a, +experimental-zvamo to enable the zvamo extension?

HsiangKai added a comment.EditedAug 9 2020, 11:07 PM

I confused the relationship of sub-extensions before. Sorry for that.
There are some discussions[0] about the relationship between vector sub-extensions. It seems that


V-extension implies Zvamo + Zvlsseg

Zvqmac, Zvamo and Zvlsseg are stand alone sub extensions. That is, Zvamo does not imply V extension.

Zvqmac has no implied extensions.
Zvamo implies A-extension.
Zvlsseg has no implied extensions.

Do you agree with that?

[0] https://github.com/riscv/riscv-v-spec/issues/546

So, let me summarize:
Zvlsseg has no implied extensions
Zvqmac has no implied extensions
Zvamo required A extension ? If so, is it suitable that use -mattr=+a, +experimental-zvamo to enable the zvamo extension?

Currently, FeatureStdExtV is the base V instructions. So, I will suggest the following relationships

Zvamo -> V (Does Zvamo -> A? I create an issue about it. https://github.com/riscv/riscv-v-spec/issues/555)
Zvqmac -> V
Zvlsseg -> V

The implementation looks like

def FeatureExtZvamo
    : SubtargetFeature<"experimental-zvamo", "HasStdExtZvamo", "true",
                       "'Zvamo'(Vector AMO Operations)",
                       [FeatureStdExtV]>;

def FeatureExtZvqmac
    : SubtargetFeature<"experimental-zvqmac", "HasStdExtZvqmac", "true",
                       "'Zvqmac' (Vector Quad-Widening Integer Multiply-Add Instructions)",
                       [FeatureStdExtV]>;

def FeatureStdExtZvlsseg
    : SubtargetFeature<"experimental-zvlsseg", "HasStdExtZvlsseg", "true",
                       "'Zvlsseg' (Vector segment load/store instructions)”,
                       [FeatureStdExtV]>;

In the future, “current V” may be changed to Zvbase in the specification. And “new V” will imply Zvbase + Zvamo + Zvlsseg. As described in https://github.com/riscv/riscv-v-spec/issues/547#issuecomment-670667964.

HsiangKai added inline comments.Aug 17 2020, 8:42 PM
llvm/lib/Target/RISCV/RISCV.td
161

There is no conclusion from 'V' specification. I think we could be conservative as Kito said. Zvamo does not imply 'A'.

Please remove FeatureStdExtA from the list. Sorry for that.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
984

Add HasStdExtA.

1022

Add HasStdExtA.

llvm/test/MC/RISCV/rvv/zvamo.s
3

--mattr=+a,experimental-zvamo

Just a couple of nits, but otherwise it LGTM.

llvm/lib/Target/RISCV/RISCVInstrFormatsV.td
330

Redundant space at the end.

llvm/lib/Target/RISCV/RISCVSchedRocket32.td
20

Rebase on master to include HasStdExtZvlsseg.

llvm/lib/Target/RISCV/RISCVSchedRocket64.td
19

Rebase on master to include HasStdExtZvlsseg.

StephenFan marked 4 inline comments as done.

rebase to latest master

HsiangKai accepted this revision.Aug 24 2020, 4:19 PM
This revision is now accepted and ready to land.Aug 24 2020, 4:19 PM
This revision was landed with ongoing or failed builds.Aug 26 2020, 11:12 PM
This revision was automatically updated to reflect the committed changes.