CodeGen: Fix pointer info in SplitVecOp_EXTRACT_VECTOR_ELT

[yaxunl]

Summary

SplitVecOp_EXTRACT_VECTOR_ELT uses dummy pointer info to generate SDLoad, which
causes isel failure on amdgcn target with amdgiz environment since amdgcn backend
needs correct address space to lower SDLoad.

This patch fixes that.

[rampitec]

LGTM

[efriedma]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

1787

You can't use "UndefValue" here; a non-null "Value*" has to be something alias analysis will understand properly.

If you want to actually get this right, you need to use MachinePointerInfo::getFixedStack(). (CreateStackTemporary doesn't explicitly return the FrameIndex, but you can extract it from the returned FrameIndexSDNode.) Or I guess we could add some way to represent an unknown pointer in a specific address-space, but that's probably not any simpler.

[yaxunl]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

1787

getFixedStack can only be used with frame index with a constant offset, but here the offset is not constant. Here we only need to carry the information about address space. Therefore undef with a specific address space. The backend won't perform any optimization based on that and only uses the address space information.

[efriedma]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

1787

getFixedStack can only be used with frame index with a constant offset, but here the offset is not constant.

Okay. You might need some other PseudoSourceValue, then.

Therefore undef with a specific address space.

undef means "this access doesn't alias anything". See BasicAAResult::aliasCheck.

The backend won't perform any optimization based on that

Among other things, we use MachinePointerInfo for scheduling... so if you get the info wrong, we could move the load before the store.

[yaxunl]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

1787

There is no existing PseudoSourceValue suitable for this.

On the other hand, undef seems to be just the right representation. This is a temporary value on stack and it is used only here and not aliased to anything else.

[efriedma]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

1787

On the other hand, undef seems to be just the right representation. This is a temporary value on stack and it is used only here and not aliased to anything else.

No, undef is clearly wrong. There is one very important aliasing relationship here: the scalar load aliases the vector store. "undef" aliases nothing. (I'm not sure the current MachineInstr::mayAlias will actually come to that conclusion in this case, but that could easily change in the future if we do more aggressive alias analysis with stack temporaries.)

[rampitec]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

1787

Scalar load from scratch is not possible as far as I understand.

[yaxunl]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

1787

How about introduce a new PseudoSourceValue kind "AddressSpace". The only known information about this pointer is its address space.

[rampitec]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

1787

To me it is an Undef in an obscure package. I think undef is just fine here. If we would use it for anything the a PseudoSourceValue holding a FI is a right choice.

[rampitec]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

1787

Also, I would say it does not make situation worse than before the change. Before it was just dummy MachinePointerInfo(). I do not really think all problems shall be solved in a single patch.

[yaxunl]

Revised by Eli's comments.

[yaxunl]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

1787

I found a way to represent a stack memory with unknown offset and have updated the patch. Please take a look. Thanks.

[yaxunl]

synch to ToT.

[yaxunl]

ping

[efriedma]

lib/CodeGen/MachineInstr.cpp

1016 ↗

(On Diff #124761)

The new patch is mostly the right idea, but I'm not sure I like the way hasUnknownOffset modifies the meaning of MachineMemOperand... we have code which accesses the offset scattered all over.

[yaxunl]

lib/CodeGen/MachineInstr.cpp

1016 ↗

(On Diff #124761)

I think what I need is a PointerInfo which is the same as PointerInfo() but has a proper address space. i.e., it returns nulptr for getValue() and getPseudoValue() but returns correct address space. This way, most of the existing codes do not need change, and the backend can still get the correct addr space.

I will see if I can implement that.

[yaxunl]

Removed member hasUnknownOffset from MachinePointerInfo and added member AddrSpace.

[efriedma]

LGTM

[foad]

llvm/trunk/include/llvm/CodeGen/MachineMemOperand.h

41–42 ↗

(On Diff #125277)

Am I right in thinking that this part of the comment should have been removed, since we're now allowing a null pointer here but with a specified (non-default) address space?

[arsenm]

llvm/trunk/include/llvm/CodeGen/MachineMemOperand.h

41–42 ↗

(On Diff #125277)

Yes

[foad]

llvm/trunk/include/llvm/CodeGen/MachineMemOperand.h

41–42 ↗

(On Diff #125277)

I removed it: 515f890033ccaad3890333513731e4c5b6db2482

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

     EltVT = MVT::i8;
     VecVT = EVT::getVectorVT(*DAG.getContext(), EltVT,
                              VecVT.getVectorNumElements());
     Vec = DAG.getBuildVector(VecVT, dl, ElementOps);
   }
 
   // Store the vector to the stack.
   SDValue StackPtr = DAG.CreateStackTemporary(VecVT);
-  SDValue Store =
-      DAG.getStore(DAG.getEntryNode(), dl, Vec, StackPtr, MachinePointerInfo());
+  auto &MF = DAG.getMachineFunction();
+  auto FrameIndex = cast<FrameIndexSDNode>(StackPtr.getNode())->getIndex();
+  auto PtrInfo = MachinePointerInfo::getFixedStack(MF, FrameIndex);
+  SDValue Store = DAG.getStore(DAG.getEntryNode(), dl, Vec, StackPtr, PtrInfo);
 
   // Load back the required element.
   StackPtr = TLI.getVectorElementPointer(DAG, StackPtr, VecVT, Idx);
   return DAG.getExtLoad(ISD::EXTLOAD, dl, N->getValueType(0), Store, StackPtr,
-                        MachinePointerInfo(), EltVT);
+                        MachinePointerInfo(UndefValue::get(Type::getInt32PtrTy(
+                            *DAG.getContext(), PtrInfo.getAddrSpace()))),
+                        EltVT);
 }
 
 SDValue DAGTypeLegalizer::SplitVecOp_ExtVecInRegOp(SDNode *N) {
   SDValue Lo, Hi;
 
   // *_EXTEND_VECTOR_INREG only reference the lower half of the input, so
   // splitting the result has the same effect as splitting the input operand.
   SplitVecRes_ExtVecInRegOp(N, Lo, Hi);

test/CodeGen/AMDGPU/extract_vector_elt-f16.ll

-; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=SI %s
-; RUN: llc -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=VI %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=SI %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=VI %s
 
 ; GCN-LABEL: {{^}}extract_vector_elt_v2f16:
 ; GCN: s_load_dword [[VEC:s[0-9]+]]
 ; GCN: s_lshr_b32 [[ELT1:s[0-9]+]], [[VEC]], 16
 ; GCN-DAG: v_mov_b32_e32 [[VELT0:v[0-9]+]], [[VEC]]
 ; GCN-DAG: v_mov_b32_e32 [[VELT1:v[0-9]+]], [[ELT1]]
 ; GCN-DAG: buffer_store_short [[VELT0]]
 ; GCN-DAG: buffer_store_short [[VELT1]]

test/CodeGen/AMDGPU/extract_vector_elt-f64.ll

-; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
-; RUN: llc -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
 
 ; GCN-LABEL: {{^}}extract_vector_elt_v3f64_2:
 ; GCN: buffer_load_dwordx4
 ; GCN: buffer_load_dwordx2
 ; GCN: buffer_store_dwordx2
 define amdgpu_kernel void @extract_vector_elt_v3f64_2(double addrspace(1)* %out, <3 x double> addrspace(1)* %in) #0 {
   %ld = load volatile <3 x double>, <3 x double> addrspace(1)* %in
   %elt = extractelement <3 x double> %ld, i32 2

test/CodeGen/AMDGPU/extract_vector_elt-i16.ll

-; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=SI -check-prefix=SICIVI %s
-; RUN: llc -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=VI -check-prefix=SICIVI %s
-; RUN: llc -march=amdgcn -mcpu=gfx901 -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=GFX9 %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=SI -check-prefix=SICIVI %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=VI -check-prefix=SICIVI %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=gfx901 -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=GFX9 %s
 
 ; GCN-LABEL: {{^}}extract_vector_elt_v2i16:
 ; GCN: s_load_dword [[VEC:s[0-9]+]]
 ; GCN: s_lshr_b32 [[ELT1:s[0-9]+]], [[VEC]], 16
 ; GCN-DAG: v_mov_b32_e32 [[VELT0:v[0-9]+]], [[VEC]]
 ; GCN-DAG: v_mov_b32_e32 [[VELT1:v[0-9]+]], [[ELT1]]
 ; GCN-DAG: buffer_store_short [[VELT0]]
 ; GCN-DAG: buffer_store_short [[VELT1]]

test/CodeGen/AMDGPU/extract_vector_elt-i64.ll

-; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
-; RUN: llc -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
 
 ; How the replacement of i64 stores with v2i32 stores resulted in
 ; breaking other users of the bitcast if they already existed
 
 ; GCN-LABEL: {{^}}extract_vector_elt_select_error:
 ; GCN: buffer_store_dword
 ; GCN: buffer_store_dword
 ; GCN: buffer_store_dwordx2

test/CodeGen/AMDGPU/extract_vector_elt-i8.ll

-; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=SI -check-prefix=FUNC %s
-; RUN: llc -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=SI -check-prefix=FUNC %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -verify-machineinstrs < %s | FileCheck -check-prefix=SI -check-prefix=FUNC %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=SI -check-prefix=FUNC %s
 
 ; FUNC-LABEL: {{^}}extract_vector_elt_v1i8:
 ; GCN: buffer_load_ubyte
 ; GCN: buffer_store_byte
 define amdgpu_kernel void @extract_vector_elt_v1i8(i8 addrspace(1)* %out, <1 x i8> %foo) #0 {
   %p0 = extractelement <1 x i8> %foo, i32 0
   store i8 %p0, i8 addrspace(1)* %out
   ret void

test/CodeGen/AMDGPU/scratch-simple.ll

-; RUN: llc -march=amdgcn -mcpu=verde -mattr=+vgpr-spilling -verify-machineinstrs < %s | FileCheck --check-prefix=GCN --check-prefix=SI %s
-; RUN: llc -march=amdgcn -mcpu=gfx804 -mattr=-flat-for-global -mattr=+vgpr-spilling -verify-machineinstrs < %s | FileCheck --check-prefix=GCN --check-prefix=SI %s
-; RUN: llc -march=amdgcn -mcpu=gfx900 -mattr=-flat-for-global -mattr=+vgpr-spilling -verify-machineinstrs < %s | FileCheck --check-prefix=GCN --check-prefix=GFX9 %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=verde -mattr=+vgpr-spilling -verify-machineinstrs < %s | FileCheck --check-prefix=GCN --check-prefix=SI %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=gfx804 -mattr=-flat-for-global -mattr=+vgpr-spilling -verify-machineinstrs < %s | FileCheck --check-prefix=GCN --check-prefix=SI %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=gfx900 -mattr=-flat-for-global -mattr=+vgpr-spilling -verify-machineinstrs < %s | FileCheck --check-prefix=GCN --check-prefix=GFX9 %s
 
 ; This used to fail due to a v_add_i32 instruction with an illegal immediate
 ; operand that was created during Local Stack Slot Allocation. Test case derived
 ; from https://bugs.freedesktop.org/show_bug.cgi?id=96602
 ;
 ; GCN-LABEL: {{^}}ps_main:
 
 ; GCN-DAG: s_mov_b32 s4, SCRATCH_RSRC_DWORD0