[DAG] Fix incorrect alignment of ext load.

[niravd]

Summary

Correctly use alignment size from loaded size not output value size.

[arsenm]

test/CodeGen/AMDGPU/extload.ll

1

Can you do something else that doesn't use -debug? This will break in release build

69

this should use a positive check

[arsenm]

Where is the call site passing 0 alignment?

[niravd]

Use different debug flag for tests

[niravd]

The 0 alignment load is coming from the splitting of the extract_vector_elt.

In D23356#511344, @arsenm wrote:

Where is the call site passing 0 alignment?

[niravd]

Ping.

[arsenm]

In D23356#523047, @niravd wrote:

The 0 alignment load is coming from the splitting of the extract_vector_elt.

In D23356#511344, @arsenm wrote:

Where is the call site passing 0 alignment?

Could you be more specific?

test/CodeGen/AMDGPU/extload.ll

1

-debug-only will have the same problem that this will only work in asserts build

[niravd]

In the SplitVecOp_EXTRACT_VECTOR_ELT(N) in LegalizeVectorTypes, there is a call to DAG.getExtLoad where the optional Alignment argument is omitted and defaulted to 0.
This appears to be the only call in llvm of such that triggers that code path and all other similar alignment checks appear correct.

In terms of a test, it's not clear to me how to expose the issue with alignment from an element of a decomposed vector in the current upstream without inspecting intermediate state via debug. The easiest way would be to fold this into D14834. I'd prefer to make this a separate commit as it is an obvious and separable mistake. Perhaps we could add this test for now, and then delete it as redundant with D14834.

In D23356#527851, @arsenm wrote:

In D23356#523047, @niravd wrote:

The 0 alignment load is coming from the splitting of the extract_vector_elt.

In D23356#511344, @arsenm wrote:

Where is the call site passing 0 alignment?

Could you be more specific?

[niravd]

Fix test so that it only runs for assert builds.

[hfinkel]

test/CodeGen/AMDGPU/extload-align.ll

5 ↗

(On Diff #70408)

It is not obvious what you're checking here. Do you want to match the alignment? (or are you matching the lack of a non-default alignment)?

[niravd]

Add comment explaining test

test/CodeGen/AMDGPU/extload-align.ll

5 ↗

(On Diff #70408)

That's correct. I'm checking that the alignment is 2 and matching the underlying i16 load, not 4 matching the size of the extend value. I've added a comment to the test making this clearer.

[niravd]

Ping.

[niravd]

Ping.

[hfinkel]

LGTM.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

SDValue SelectionDAG::getLoad(ISD::MemIndexedMode AM, ISD::LoadExtType ExtType,
                               SDValue Ptr, SDValue Offset,
                               MachinePointerInfo PtrInfo, EVT MemVT,
                               unsigned Alignment,
                               MachineMemOperand::Flags MMOFlags,
                               const AAMDNodes &AAInfo, const MDNode *Ranges) {
   assert(Chain.getValueType() == MVT::Other &&
         "Invalid chain type");
   if (Alignment == 0)  // Ensure that codegen never sees alignment 0
-    Alignment = getEVTAlignment(VT);
+    Alignment = getEVTAlignment(MemVT);
 
   MMOFlags |= MachineMemOperand::MOLoad;
   assert((MMOFlags & MachineMemOperand::MOStore) == 0);
   // If we don't have a PtrInfo, infer the trivial frame index case to simplify
   // clients.
   if (PtrInfo.V.isNull())
     PtrInfo = InferPointerInfo(*this, Ptr, Offset);
 

test/CodeGen/AMDGPU/extload.ll

-; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=SI-NOHSA -check-prefix=FUNC %s
+; RUN: llc -debug-only=misched -march=amdgcn -verify-machineinstrs %s -o - 2>&1| FileCheck -check-prefix=SI-NOHSA -check-prefix=FUNC -check-prefix=DEBUG %s
 ; RUN: llc -mtriple=amdgcn-amdhsa -mcpu=kaveri -verify-machineinstrs < %s | FileCheck -check-prefix=FUNC -check-prefix=CI-HSA -check-prefix=SI %s
 ; RUN: llc -march=amdgcn -mcpu=tonga -verify-machineinstrs < %s | FileCheck -check-prefix=SI-NOHSA -check-prefix=FUNC %s
 ; RUN: llc -march=r600 -mcpu=redwood < %s | FileCheck -check-prefix=EG -check-prefix=FUNC %s
 
 ; FIXME: This seems to not ever actually become an extload
 ; FUNC-LABEL: {{^}}global_anyext_load_i8:
 ; GCN: buffer_load_dword v{{[0-9]+}}
 ; GCN: buffer_store_dword v{{[0-9]+}}

 define void @local_anyext_load_i16(i16 addrspace(3)* nocapture noalias %out, i16 addrspace(3)* nocapture noalias %src) nounwind {
   %cast = bitcast i16 addrspace(3)* %src to i32 addrspace(3)*
   %load = load i32, i32 addrspace(3)* %cast
   %x = bitcast i32 %load to <2 x i16>
   %castOut = bitcast i16 addrspace(3)* %out to <2 x i16> addrspace(3)*
   store <2 x i16> %x, <2 x i16> addrspace(3)* %castOut
   ret void
 }
+
+
+; DEBUG: {{^}}# Machine code for function extload_align:
+; DEBUG: mem:LD2[<unknown>]{{[^(]}}
+; DEBUG: {{^}}# End machine code for function extload_align.
+
+define void @extload_align(i32* %out, i32 %index) #0 {
+  %v0 = alloca [4 x i16]
+  %a1 = getelementptr inbounds [4 x i16], [4 x i16]* %v0, i32 0, i32 0
+  %a2 = getelementptr inbounds [4 x i16], [4 x i16]* %v0, i32 0, i32 1
+  store i16 0, i16* %a1
+  store i16 1, i16* %a2
+  %a = getelementptr inbounds [4 x i16], [4 x i16]* %v0, i32 0, i32 %index
+  %val = load i16, i16* %a
+  %eval = sext i16 %val to i32
+  store i32 %eval, i32* %out
+  ret void
+}
 No newline at end of file