Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV][GISel] Legalize G_SMULO/G_UMULO #67635

Merged
merged 5 commits into from
Oct 14, 2023
Merged

[RISCV][GISel] Legalize G_SMULO/G_UMULO #67635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
843549e
[RISCV][GISel] Legalize G_SMULO/G_UMULO
topperc Sep 28, 2023
56b7ccb
!fixup reduce code duplication
topperc Sep 29, 2023
b2137ec
!fixup clang-format
topperc Sep 29, 2023
9132427
Fixes to get a single libcall for XLenLLT.
topperc Sep 30, 2023
2ca370d
!fixup remove unused lambda capture
topperc Oct 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1995,8 +1995,20 @@ LegalizerHelper::widenScalarMulo(MachineInstr &MI, unsigned TypeIdx,
auto LeftOperand = MIRBuilder.buildInstr(ExtOp, {WideTy}, {LHS});
auto RightOperand = MIRBuilder.buildInstr(ExtOp, {WideTy}, {RHS});

auto Mulo = MIRBuilder.buildInstr(MI.getOpcode(), {WideTy, OverflowTy},
{LeftOperand, RightOperand});
// Multiplication cannot overflow if the WideTy is >= 2 * original width,
// so we don't need to check the overflow result of larger type Mulo.
bool WideMulCanOverflow = WideTy.getScalarSizeInBits() < 2 * SrcBitWidth;

unsigned MulOpc =
WideMulCanOverflow ? MI.getOpcode() : (unsigned)TargetOpcode::G_MUL;

MachineInstrBuilder Mulo;
if (WideMulCanOverflow)
Mulo = MIRBuilder.buildInstr(MulOpc, {WideTy, OverflowTy},
{LeftOperand, RightOperand});
else
Mulo = MIRBuilder.buildInstr(MulOpc, {WideTy}, {LeftOperand, RightOperand});

auto Mul = Mulo->getOperand(0);
MIRBuilder.buildTrunc(Result, Mul);

Expand All @@ -2014,9 +2026,7 @@ LegalizerHelper::widenScalarMulo(MachineInstr &MI, unsigned TypeIdx,
ExtResult = MIRBuilder.buildZExtInReg(WideTy, Mul, SrcBitWidth);
}

// Multiplication cannot overflow if the WideTy is >= 2 * original width,
// so we don't need to check the overflow result of larger type Mulo.
if (WideTy.getScalarSizeInBits() < 2 * SrcBitWidth) {
if (WideMulCanOverflow) {
auto Overflow =
MIRBuilder.buildICmp(CmpInst::ICMP_NE, OverflowTy, Mul, ExtResult);
// Finally check if the multiplication in the larger type itself overflowed.
Expand Down
18 changes: 18 additions & 0 deletions llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,13 +145,31 @@ RISCVLegalizerInfo::RISCVLegalizerInfo(const RISCVSubtarget &ST) {
.legalFor({XLenLLT})
.lower();
// clang-format on

getActionDefinitionsBuilder({G_SMULO, G_UMULO})
Copy link

@tschuett tschuett Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am maybe blind, but could you hoist them above the if? They look similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah they're the same. They were briefly different while I tried and failed to fix the 2 libcall issue.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling that you are going for coverage and not performance. Could you instead do a custom legalization, i.e. RISC-V specific and not generic lower?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The widening of small types creates an XLenLLT sized G_SMULO even though that can't overflow if the small type was half of XLen or less. I don't know how to distinquish that G_SMULO from the original. So I ended up with a double XLen libcall no matter what which is not really what I wanted.

I guess I could write custom legalization for everything, but didn't seem ideal.

.minScalar(0, XLenLLT)
.lower();
} else {
getActionDefinitionsBuilder(G_MUL)
.libcallFor({XLenLLT, DoubleXLenLLT})
.widenScalarToNextPow2(0)
.clampScalar(0, XLenLLT, DoubleXLenLLT);

getActionDefinitionsBuilder({G_SMULH, G_UMULH}).lowerFor({XLenLLT});

getActionDefinitionsBuilder({G_SMULO, G_UMULO})
.minScalar(0, XLenLLT)
// Widen XLenLLT to DoubleXLenLLT so we can use a single libcall to get
// the low bits for the mul result and high bits to do the overflow
// check.
.widenScalarIf(
[=](const LegalityQuery &Query) {
return Query.Types[0] == XLenLLT;
},
[=](const LegalityQuery &Query) {
return std::make_pair(0, DoubleXLenLLT);
})
.lower();
}

if (ST.hasStdExtM()) {
Expand Down
Loading