[PATCH] D112395: [AMDGPU] Enable 48-bit mul in AMDGPUCodeGenPrepare.

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Seems reasonable, just some nits inline.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:487
+
+  if (Size <= 32 || (!IsSigned && NumBits <= 32) ||
+      (IsSigned && NumBits <= 30)) {
----------------
This would be neater with `IsSigned ? ... : ...`


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:494
+
+  assert((!IsSigned && NumBits <= 48) || (IsSigned && NumBits <= 46));
+
----------------
This would be neater with `IsSigned ? ... : ...`


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:502
+  Value *Lo =
+      Builder.CreateCall(Intrinsic::getDeclaration(Mod, LoID), {LHS, RHS});
+  Value *Hi =
----------------
Can you use CreateIntrinsic to create all the calls?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:534
 
   if (ST->hasMulU24() && (LHSBits = numBitsUnsigned(LHS, Size)) <= 24 &&
       (RHSBits = numBitsUnsigned(RHS, Size)) <= 24) {
----------------
Do all subtargets that have mul_u24 also have mulhi_u24, and the same for i24?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:555
     Value *LHS, *RHS;
-    if (IntrID == Intrinsic::amdgcn_mul_u24) {
+    if (!IsSigned) {
       LHS = Builder.CreateZExtOrTrunc(LHSVals[I], I32Ty);
----------------
I prefer not to have a negated condition for an "if" if there's an "else" as well, otherwise the condition for the "else" is a double negative which is harder to understand.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:566
 
-    if (IntrID == Intrinsic::amdgcn_mul_u24) {
+    if (!IsSigned) {
       ResultVals.push_back(Builder.CreateZExtOrTrunc(Result,
----------------
Same.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112395/new/

https://reviews.llvm.org/D112395