[PATCH] D140208: [AMDGPU] Improved wide multiplies

[Jessica Del via Phabricator via llvm-commits]

OutOfCache marked an inline comment as done.
OutOfCache added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3040
             unsigned j1 = DstIndex - j0;
+            if (Src0KnownZeros[j0] || Src1KnownZeros[j1]) {
+              ++j0;
----------------
tsymalla wrote:
> Can one of these accesses be out-of-bounds now that you removed the assumption that both Src0 and Src1 are of equal length?
I double-checked and the way the code works currently makes sure that both `Src0` and `Src1` are the same length. In line 3179, both arrays are created in the same for-loop. I don't know if there is a case where they will have different sizes. If the operands are not of the same size, an error is thrown and it does not compile. So in theory yes, but in practice no? Is this a case that should be considered or would that be too much?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2892-2897
+void AMDGPULegalizerInfo::buildMultiply(LegalizerHelper &Helper,
+                                        MutableArrayRef<Register> Accum,
+                                        ArrayRef<Register> Src0,
+                                        ArrayRef<Register> Src1,
+                                        bool UsePartialMad64_32,
+                                        bool SeparateOddAlignedProducts) const {
----------------
tsymalla wrote:
> OutOfCache wrote:
> > nhaehnle wrote:
> > > Did you run clang-format on this? Usually it chooses a different layout.
> > I did. It even caused failing pre-merge checks if I remember correctly. I ran clang-format again and this is still the result.
> You can try clang-format -style=file:path and see if this changes something. I think you can also manually invoke git-clang-format to see if anything would have been changed by clang-format.
I ran `git clang-format` but it didn't change this part. `clang-format file -i` changed the file in a lot of places, but not here either. Maybe someone could double-check on their machine?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140208