[PATCH] D140208: [AMDGPU] Improved wide multiplies

[Thomas Symalla via Phabricator via llvm-commits]

tsymalla added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3181
 
+  const bool Src0IsZero = Helper.getKnownBits()->getKnownBits(Src0).isZero();
+  const bool Src1IsZero = Helper.getKnownBits()->getKnownBits(Src1).isZero();
----------------
Can be merged to `AtLeastOneSrcIsZero` (just like with `AtLeastOneArgIsZero`)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3187
+  } else {
   LLT S32 = LLT::scalar(32);
   SmallVector<Register, 2> Src0Parts, Src1Parts;
----------------
Intendation (maybe run clang-format)?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/mul-known-bits.i64.ll:6
+; A 64-bit multiplication where no arguments were zero extended.
+define amdgpu_kernel void @v_mul_i64_zext_00(i64 addrspace(1)* %out, i64 addrspace(1)* %aptr, i64 addrspace(1)* %bptr) nounwind {
+; FUNC-LABEL: v_mul_i64_zext_00:
----------------
Can you give some more insightful names to the tests (appending _xy vs. naming the test `@v_mul_i64_zext_no_args` or something similar)?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/mul-known-bits.i64.ll:7
+define amdgpu_kernel void @v_mul_i64_zext_00(i64 addrspace(1)* %out, i64 addrspace(1)* %aptr, i64 addrspace(1)* %bptr) nounwind {
+; FUNC-LABEL: v_mul_i64_zext_00:
+; FUNC:       ; %bb.0:
----------------
Can you pre-commit the tests so it is easier to identify the changes in the assembly?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/mul-known-bits.i64.ll:146
+  %b = load i64, i64 addrspace(1)* %gep.b
+  %a_and = and i64 %a, u0x00000000FFFFFFFF
+  %mul = mul i64 %a_and, %b
----------------
Maybe add some test to show whats happening when both high and low bits are being masked (0x0000FFFF000...)


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/mul-known-bits.i64.ll:235
+; 64-bit multiplication, where the first argument is masked before a branch
+define amdgpu_kernel void @mul64_and_in_branch(i64 addrspace(1)* %out, i64 addrspace(1)* %aptr, i64 addrspace(1)* %bptr) {
+; FUNC-LABEL: mul64_and_in_branch:
----------------
Should this tests and the ones following also be prefixed with `v_`?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/mul-known-bits.i64.ll:270
+
+; 64-bit multiplication with both arguments changed in differnt basic blocks.
+define amdgpu_kernel void @mul64_and_in_branch_2(i64 addrspace(1)* %out, i64 addrspace(1)* %aptr, i64 addrspace(1)* %bptr) {
----------------
Typo: differnt


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/mul-known-bits.i64.ll:272
+define amdgpu_kernel void @mul64_and_in_branch_2(i64 addrspace(1)* %out, i64 addrspace(1)* %aptr, i64 addrspace(1)* %bptr) {
+; FUNC-LABEL: mul64_and_in_branch_2:
+; FUNC:       ; %bb.0: ; %entry
----------------
Maybe add some 32-bit tests to show that your changes are being applied correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140208