This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][GlobalISel] Legalize all ALU instructions, excluding w-instructions
AbandonedPublic

Authored by nitinjohnraj on Jun 12 2023, 9:50 AM.

Details

Summary

We legalize the ALU instructions for RISCV according to the hardware mode. W-instructions will be supported in a future patch.

Diff Detail

Event Timeline

nitinjohnraj created this revision.Jun 12 2023, 9:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2023, 9:50 AM
nitinjohnraj requested review of this revision.Jun 12 2023, 9:50 AM
nitinjohnraj edited the summary of this revision. (Show Details)Jun 12 2023, 9:59 AM
nitinjohnraj added a reviewer: lewis-revill.

I've added the previous patch legalizing ALU instructions to a child revision of this, since it also attempts to handle w-instructions. Our first major goal is implementing globalisel at -O0; w-instructions are not needed yet.

llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
61

Should we manually edit the formatting here?

As long as you are not interested in vectors, I used this pattern:

getActionDefinitionsBuilder(G_FREEZE)
    .legalFor({s8, s16, s32, s64, p0})
    .widenScalarToNextPow2(0, /*Min=*/8)
    .clampScalar(0, s8, sMaxScalar);

clamp to legal range and widen to next power of two to handle weird cases.

craig.topper added inline comments.Jun 12 2023, 10:57 AM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
40

I don't see any command lines with -mattr=+m, so this branch isn't tested?

In fact there are no test cases for mul, udiv, urem, sdiv, or srem.

nitinjohnraj planned changes to this revision.Jun 13 2023, 12:44 PM

Add tests for smaller sizes, mul/div/rem variants.

llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
40

You're right, I'll add these.

tschuett added inline comments.Jun 13 2023, 12:48 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
49

What is the effect of the clampScalar? You are doing lib call for legal values?

Fixed formatting and added tests for smaller/larger sizes on RV32.

As long as you are not interested in vectors, I used this pattern:

getActionDefinitionsBuilder(G_FREEZE)
    .legalFor({s8, s16, s32, s64, p0})
    .widenScalarToNextPow2(0, /*Min=*/8)
    .clampScalar(0, s8, sMaxScalar);

clamp to legal range and widen to next power of two to handle weird cases.

Thank you for the comment, I formatted the code similarly.

arsenm added inline comments.Jun 14 2023, 6:54 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
47–48

I don't think ordering libcall before anything else will work

Broke tests into individual files and reordered some rules

craig.topper added inline comments.Jul 5 2023, 9:57 AM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
53

Is this libcall tested? There is no s128 libcall for RV32, but s64 should use a libcall on RV64.

77

G_CONSTANT should only have 1 type. What does .maxScalar(1, XLenLLT); here do?

80

How can G_ZEXT/G_SEXT/G_ANYEXT be legal for any types? The source and destination should be different types, and there is only 1 legal type on RISC-V.

tschuett added inline comments.Jul 6 2023, 12:10 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
80
arsenm added inline comments.Jul 6 2023, 12:22 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
80

I think the AArch64 rules are over-permissive and old. It shouldn't be permitting <= register size, only == register size values through

tschuett added inline comments.Jul 6 2023, 12:25 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
80

I found remark: unable to legalize instruction: %252:_(<16 x s32>) = G_ZEXT %249:_(<16 x s8>) [-Rpass-missed=gisel-legalize] in the wild. Till today, I have no clue how to legalize it on AArch64.

nitinjohnraj abandoned this revision.Jul 11 2023, 8:50 AM
nitinjohnraj marked 4 inline comments as done.

Breaking the problem into smaller chunks seems helpful.

llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
47–48

I moved the libcall after the clampScalar, but retrospectively I think the original ordering was correct.

getActionDefinitionsBuilder({G_MUL, G_SDIV, G_SREM, G_UDIV, G_UREM})
          .legalFor({XLenLLT})
          .libcallFor({s128})
          .clampScalar(0, XLenLLT, XLenLLT);

This reads as "These opcodes are legal for XLenLLT. If that check fails, see if the type is s128 and then use a libcall. If that fails, try extending the argument 0 to XLenLLT type." That seems correct to me.

49

If I understand correctly, this is being addressed by the conversation with arsenm above.