This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add codegen support for RISCV XVentanaCondOps Extension
ClosedPublic

Authored by kconsul on Dec 5 2022, 11:04 PM.

Details

Summary

This patch adds codegen support for part of XVentanaCondOps extension.
This extension is designed to reduce the number of branches in
the generated RISCV assembly by replacing branches with conditional
move instructions as defined by XVentanaCondOps specification.

The specification for XVentanaCondOps extension can be found at:
https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.1/ventana-custom-extensions-v1.0.1.pdf

Signed-off-by: Kautuk Consul <kconsul@ventanamicro.com>
Signed-off-by: Mikhail Gudim <mgudim@ventanamicro.com>

Diff Detail

Event Timeline

kconsul created this revision.Dec 5 2022, 11:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2022, 11:04 PM
kconsul requested review of this revision.Dec 5 2022, 11:04 PM
jrtc27 added inline comments.Dec 5 2022, 11:10 PM
llvm/test/CodeGen/RISCV/xventanacondops.ll
2

... why MIR, just do asm like a normal CodeGen test

reames requested changes to this revision.Dec 6 2022, 7:48 AM
reames added a subscriber: reames.

As @jrtc27 mentions, we already have in tree support for XVentanCondOps. This was added in D137350.

Your change does include a number of things which are not currently in tree. Please rebase this patch, and adjust the description to reflect the changes that remain please.

This revision now requires changes to proceed.Dec 6 2022, 7:48 AM
kconsul updated this revision to Diff 480849.Dec 7 2022, 4:17 AM

[RISCV] Add support for RISCV XVentanaCondops Extension

This patch adds support for part of XVentanaCondops extension.
This extension is designed to reduce the number of branches in
the generated RISCV assembly by replacing branches with conditional
move instructions as defined by XVentanaCondops specification.

The specification for XVentanaCondops extension can be found at:
https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.1/ventana-custom-extensions-v1.0.1.pdf

Signed-off-by: Kautuk Consul <kconsul@ventanamicro.com>
Signed-off-by: Mikhail Gudim <mgudim@ventanamicro.com>

Changes since v1:

  • Rebased on the main branch.
  • Ported my MASKC/MASKCN changes to VT_MASKC/VT_MASKCN as per Philip Reames' patch which added assembler support for the vt.maskc/vt.maskcn instructions.
craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4510

This doesn't belong here. It should be done on setOperationAction call that made ISD::SELECT Custom.

I think it's also incorrect here because it disabled SELECT_CC formation for FP selects.

llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
58

Missing space before (VT_MASKCN

llvm/test/CodeGen/RISCV/xventanacondops.ll
3

Why stop-after finalize-isel?

llvm/test/MC/RISCV/rv64xventanacondops-valid.s
1 ↗(On Diff #480849)

don't we already have a test called XVentanaCondOps-valid.s?

asb added a comment.Dec 9 2022, 2:45 AM

Leaving a comment here for completeness, as I reported back in the other RISC-V extension patches that were put on the agenda for the RISC-V sync call yesterday. We already support these condops at the MC level, so the discussion was mostly focused around the practicalities of getting this patch in a form that can be merged and determining what plans people have for any related follow-up optimisations.

kconsul updated this revision to Diff 481600.Dec 9 2022, 4:19 AM

[RISCV] Add support for RISCV XVentanaCondops Extension

This patch adds support for part of XVentanaCondops extension.
This extension is designed to reduce the number of branches in
the generated RISCV assembly by replacing branches with conditional
move instructions as defined by XVentanaCondops specification.

The specification for XVentanaCondops extension can be found at:
https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.1/ventana-custom-extensions-v1.0.1.pdf

Signed-off-by: Kautuk Consul <kconsul@ventanamicro.com>
Signed-off-by: Mikhail Gudim <mgudim@ventanamicro.com>

Changes since v2:

  • Inserted a space before "(VT_MASKCN"
  • Removed the xventanacondops-valid.s and xventanacondops-invalid.s
  • Modified the code in RISCVTargetLowering::RISCVTargetLowering() so that the setOperationAction for ISD::SELECT is called only when xvenatancondops attribute is not set.
  • Modified the xventanacondops.ll test-case to check till full asm.
craig.topper added inline comments.Dec 9 2022, 3:10 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
309–310

Use !Subtarget.hasVendorXVentanaCondOps()

llvm/test/CodeGen/RISCV/xventanacondops.ll
3

Drop --check-prefix and use the default CHECK

kconsul updated this revision to Diff 482004.Dec 11 2022, 11:45 PM

[RISCV] Add support for RISCV XVentanaCondops Extension

This patch adds support for part of XVentanaCondops extension.
This extension is designed to reduce the number of branches in
the generated RISCV assembly by replacing branches with conditional
move instructions as defined by XVentanaCondops specification.

The specification for XVentanaCondops extension can be found at:
https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.1/ventana-custom-extensions-v1.0.1.pdf

Signed-off-by: Kautuk Consul <kconsul@ventanamicro.com>
Signed-off-by: Mikhail Gudim <mgudim@ventanamicro.com>

Changes since v3:

  • use Subtarget.hasVendorXVentanaCondOps() in the check in llvm/lib/Target/RISCV/RISCVISelLowering.cpp.
  • Drop the -check-prefix=RV64 argument to FileCheck
kconsul updated this revision to Diff 482028.EditedDec 12 2022, 1:29 AM

This patch adds support for part of XVentanaCondops extension.
This extension is designed to reduce the number of branches in
the generated RISCV assembly by replacing branches with conditional
move instructions as defined by XVentanaCondops specification.

The specification for XVentanaCondops extension can be found at:
https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.1/ventana-custom-extensions-v1.0.1.pdf

Changes since v4:

  • Modified the CodeGen/RISCV/select.ll test-case for the implementation of xventanacondops feature in RISCV backend.
craig.topper accepted this revision.Dec 13 2022, 11:58 AM

LGTM with the extra RV64 lines removed from the test.

llvm/test/CodeGen/RISCV/xventanacondops.ll
6

Please remove the stale RV64 lines.

kconsul updated this revision to Diff 482727.Dec 13 2022, 11:57 PM

[RISCV] Add support for RISCV XVentanaCondops Extension patterns

This patch adds support for XVentanaCondops extension patterns.
This extension is designed to reduce the number of branches in
the generated RISCV assembly by replacing branches with conditional
move instructions as defined by XVentanaCondops specification.

The specification for XVentanaCondops extension can be found at:
https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.1/ventana-custom-extensions-v1.0.1.pdf

Signed-off-by: Kautuk Consul <kconsul@ventanamicro.com>
Signed-off-by: Mikhail Gudim <mgudim@ventanamicro.com>

Changes since v5:

  • Removed the RV64 lines from the xventanacondops.ll test-case.

Subject and first line of the description should be clear it's about code generation. As it stands it still sounds like you're adding MC support.

Subject and first line of the description should be clear it's about code generation. As it stands it still sounds like you're adding MC support.

I have changed the description to state that I am adding support for xventanacondops patterns.

llvm/test/CodeGen/RISCV/xventanacondops.ll
2

I have implemented changes in RISCV backend so that llc generates these instructions for the select pattern. So to test those patterns we would need MIR right ?

kconsul retitled this revision from [RISCV] Add support for RISCV XVentanaCondops Extension to [RISCV] Add support for RISCV XVentanaCondops Extension Patterns.Dec 14 2022, 10:06 PM
kconsul edited the summary of this revision. (Show Details)
jrtc27 added inline comments.Dec 14 2022, 10:10 PM
llvm/test/CodeGen/RISCV/xventanacondops.ll
2

Normally you just test the asm output like you’re doing now

"Ops" in "XVentanaCondops" is capitalized in the specification and the code, but not in your patch title or patch description

kconsul edited the summary of this revision. (Show Details)Dec 14 2022, 10:43 PM
kconsul retitled this revision from [RISCV] Add support for RISCV XVentanaCondops Extension Patterns to [RISCV] Add support for RISCV XVentanaCondOps Extension Patterns.Dec 14 2022, 11:50 PM
kconsul edited the summary of this revision. (Show Details)

"Ops" in "XVentanaCondops" is capitalized in the specification and the code, but not in your patch title or patch description

Done.

It's not really clear to me that the lowering here is what we want in general. This is certainly more general than the branchless lowerings previously implemented, but at least in several cases, it looks like the branchless lowerings would be better. I've also tagged a bunch of non-optimal lowering cases which the current branchless lowerings don't catch.

llvm/test/CodeGen/RISCV/select.ll
25

Using condops in this case appears to be a regression.

287

We should be able to do better here since 0 is the identity constant for the add.

We should be able to lower this as (add (select cond, a, 0), b). There should only need a single vt.maskc here.

423

Same here with the identity constant observation.

549

We should be able to:
li t1, 43
vt.maskc t1, t1, t0 0 or 43
sub t1, t1, 1
-1 or 42
and a0, a1, t1 // result

796

ashr a, (select cond, %b, 0) would likely be a better expansion.

craig.topper added inline comments.Dec 15 2022, 3:05 PM
llvm/test/CodeGen/RISCV/select.ll
287

InstCombine already does that transform before we get to SelectionDAG so it might not be an issue in practice.

craig.topper added inline comments.Dec 15 2022, 3:31 PM
llvm/test/CodeGen/RISCV/select.ll
287

Note there are two operations built into the addw. An add and a sign extension.

Doing a peephole on this in isel would require knowing that the promoted version of %b has 33 sign bits so that addw %b, 0 was equivalent to %b. It doesn't have 33 sign bits because it doesn't have the signext attribute.

It would also be valid if we knew the upper bits of the result didn't matter. But we have lost that information by the time we reach isel.

Alternatively, we could rewrite it before type legalization if we thought it was important.

423

Same issue as the addw above

reames added inline comments.Dec 15 2022, 4:40 PM
llvm/test/CodeGen/RISCV/select.ll
287

When making the original comment I had not realized this was specific to i32, and that the i64 case looks better below.

Hm, this points out a testing issue here. All of the existing tests are i32, and the added ones are i64. We should probably merge the test files so that the native width codegen is visible with and without.

To be clear, this is not meant to be a blocking comment. More making observations on potential improvements.

craig.topper retitled this revision from [RISCV] Add support for RISCV XVentanaCondOps Extension Patterns to [RISCV] Add codegen support for RISCV XVentanaCondOps Extension.Dec 15 2022, 11:21 PM
craig.topper edited the summary of this revision. (Show Details)

I modified the description and title to use "codegen support" instead of "patterns" that better matches the recent extension patches like Zca.

I think we can commit this and work on improvements in separate patches.

I think we can commit this and work on improvements in separate patches.

Do I do a "git push" for this or will one of you do this for me ?
Sorry but I am new to LLVM reviews.llvm.org procedures for committing a change.

I think we can commit this and work on improvements in separate patches.

Do I do a "git push" for this or will one of you do this for me ?
Sorry but I am new to LLVM reviews.llvm.org procedures for committing a change.

If you don't have commit access. I can push it. Should I use "Kautuk Consul <kconsul@ventanamicro.com>" for the commit author?

I think we can commit this and work on improvements in separate patches.

Do I do a "git push" for this or will one of you do this for me ?
Sorry but I am new to LLVM reviews.llvm.org procedures for committing a change.

If you don't have commit access. I can push it. Should I use "Kautuk Consul <kconsul@ventanamicro.com>" for the commit author?

That and "Mikhail Gudim <mgudim@ventanamicro.com>" because I had help from him on this. Thanks!

craig.topper added inline comments.Dec 18 2022, 11:28 PM
llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
52

I think the VT_MASKC and VT_MASKCN are swapped in these patterns?

llvm/test/CodeGen/RISCV/xventanacondops.ll
198

I believe this corresponds to "Conditional bitwise-and, if non-zero" in the spec which should have this assembly

and rd, rs1, rs2
vt.maskcn rtmp, rs1, rc
or rd, rd, rtmp
craig.topper requested changes to this revision.Dec 18 2022, 11:28 PM
This revision now requires changes to proceed.Dec 18 2022, 11:28 PM
kconsul updated this revision to Diff 483935.EditedDec 19 2022, 5:58 AM

[RISCV] Add codegen support for RISCV XVentanaCondOps Extension

This patch adds codegen support for part of XVentanaCondops extension.
This extension is designed to reduce the number of branches in
the generated RISCV assembly by replacing branches with conditional
move instructions as defined by XVentanaCondops specification.

The specification for XVentanaCondops extension can be found at:
https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.1/ventana-custom-extensions-v1.0.1.pdf

Signed-off-by: Kautuk Consul <kconsul@ventanamicro.com>
Signed-off-by: Mikhail Gudim <mgudim@ventanamicro.com>

Changes since v6:

  • Swapped the VT_MASKC and VT_MASKCN in the conditional AND patterns.
  • Modified the xventanacondops.ll test-case with respect to the conditional AND pattern change.
  • Modified the select.ll test-case with respect to the conditional AND pattern change.
This revision was not accepted when it landed; it landed in state Needs Review.Dec 19 2022, 10:08 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.