[PATCH] D85653: [GlobalISel] widenScalar G_SMULH/G_UMULH
Matt Arsenault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 10 12:41:09 PDT 2020
arsenm added inline comments.
================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1764
+ bool IsSigned = MI.getOpcode() == TargetOpcode::G_SMULH;
+ auto ExtOp = IsSigned ? TargetOpcode::G_SEXT : TargetOpcode::G_ZEXT;
+
----------------
no auto.
Can't this use anyext?
================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1768
+ auto RHS = MIRBuilder.buildInstr(ExtOp, {WideTy}, {MI.getOperand(2)});
+ auto Mul = MIRBuilder.buildMul(WideTy, LHS, RHS);
+ auto Result = MI.getOperand(0);
----------------
Something seems off to me about introducing a full multiply, and in whatever type the user requested. I think this only works if WideTy == 2 * OriginalType. Can you produce a mulh in the wider type? This seems more like a lowering
================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1769
+ auto Mul = MIRBuilder.buildMul(WideTy, LHS, RHS);
+ auto Result = MI.getOperand(0);
+
----------------
Use Register, I would worry about introducing a copy of MachineOperand here
================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1771
+
+ auto Ty = MRI.getType(MI.getOperand(0).getReg());
+ auto Size = Ty.getScalarSizeInBits();
----------------
LLT not auto
================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1775
+
+ auto Shamt = MIRBuilder.buildConstant(WideTy, Size - IsSigned);
+ auto Shifted = MIRBuilder.buildInstr(ShiftOp, {WideTy}, {Mul, Shamt});
----------------
ShiftAmt?
================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1775
+
+ auto Shamt = MIRBuilder.buildConstant(WideTy, Size - IsSigned);
+ auto Shifted = MIRBuilder.buildInstr(ShiftOp, {WideTy}, {Mul, Shamt});
----------------
arsenm wrote:
> ShiftAmt?
Why isn't the shift amount WideTy.getSizeInBits() - Size? I don't understand - IsSigned
================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1819
}
+
+ case TargetOpcode::G_UMULH:
----------------
Extra newline
================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-smulh.mir:68
+ $vgpr0 = COPY %5
+...
----------------
Can you add an 8 and 24-bit test?
================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-umulh.mir:109
+ $vgpr0 = COPY %5
+...
----------------
Can you add an 8 and 24-bit test?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85653/new/
https://reviews.llvm.org/D85653
More information about the llvm-commits
mailing list