[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:3040
             unsigned j1 = DstIndex - j0;
+            if (Src0KnownZeros[j0] || Src1KnownZeros[j1]) {
+              ++j0;
----------------
OutOfCache wrote:
> 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?
@nhaehnle If both arrays are initialized in one loop, shouldn't we just move back to initialize `Src0KnownZeros` and `Src1KnownZeros` in one loop, too?


================
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 {
----------------
OutOfCache wrote:
> 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?
I just re-ran `git-clang-format HEAD~1` and it did not change anything for me.  The Buildbot does not seem to spill out an error due to clang-format, so I guess it's alright. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140208