This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][GlobalISel] Legalize multiplication
ClosedPublic

Authored by nitinjohnraj on Aug 8 2023, 10:00 AM.

Details

Summary

Legalize multiplication with the +m, +zmmul extensions and without extensions. With extensions, we test for (s7, s8, s16, s32, s48, s64, s96) on rv32 and (s8, s15, s32, s64, s72, s128, s192) on rv64. Without extensions, test (s7, s8, s16, s32) on rv32 and (s8, s15, s16, s32, s64) on rv64. Does not yet work for the type which is 2 times XLen without extensions.

Diff Detail

Event Timeline

nitinjohnraj created this revision.Aug 8 2023, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 10:00 AM

Fixed legalization for 2 x XLen without extensions

nitinjohnraj published this revision for review.Aug 8 2023, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 1:57 PM
craig.topper added inline comments.Aug 8 2023, 11:23 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
111

Why did we stray from the usual formatting here?

llvm/test/CodeGen/RISCV/GlobalISel/legalizer/rv32/legalize-mul-ext.mir
6

Can +m and +zmmul share the same check prefix?

11

CHECK-I is not a valid prefix for this test.

llvm/test/CodeGen/RISCV/GlobalISel/legalizer/rv64/legalize-mul-ext.mir
4

Check prefixes in this test have an I in them that the rv32 tests don't have. We should be consistent

nitinjohnraj marked 3 inline comments as done.Aug 10 2023, 9:09 AM
nitinjohnraj added inline comments.
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
111

clang-format does this. You can see similar formatting on line 71. Should I manually format it with // clang-format off tags?

Fixed tests to use same prefix

craig.topper added inline comments.Aug 10 2023, 9:37 AM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
111

I support turning off clang-format here

craig.topper added inline comments.Aug 10 2023, 9:38 AM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
111

Why do we need .lower() on this? We have instructions for G_SMULH and G_UMULH.

arsenm added inline comments.Aug 10 2023, 9:40 AM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
111

I would assume you want some clamping before going direct to lower (although I wouldn't be surprised if there wasn't complete support for mulh lowering)

craig.topper added inline comments.Aug 10 2023, 10:58 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
111

Ignore my comment. I think I missed the legalFor

tschuett added inline comments.
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
106

This builder looks the same with and without the extension? Could you hoist it above the if?

craig.topper added inline comments.Aug 11 2023, 9:22 AM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
106

Is the MULO part of this tested?

Fixed formatting and deleted legalization for G_(U/S)MULO. G_(U/S)MULO needs to be legalized in another patch with its own tests.

craig.topper added inline comments.Aug 15 2023, 1:18 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
117

I assume MULO here isn't needed/tested either?

Removed MULO rules missed in previous update

This revision is now accepted and ready to land.Aug 15 2023, 6:25 PM
This revision was landed with ongoing or failed builds.Aug 17 2023, 1:00 PM
This revision was automatically updated to reflect the committed changes.