[PATCH 3/5] R600: Convert non global value const loads to global loads
Jan Vesely
jan.vesely at rutgers.edu
Tue May 26 14:34:41 PDT 2015
On Mon, 2015-05-25 at 19:07 -0700, Matt Arsenault wrote:
> > On May 25, 2015, at 5:08 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
> >
> > This seems more complicated, but const laods are getting their own instructions
> > in the next commit
>
> Typo laods in message
>
> >
> > Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu <mailto:jan.vesely at rutgers.edu>>
> > ---
> > lib/Target/R600/AMDGPUISelDAGToDAG.cpp | 4 ++--
> > lib/Target/R600/R600ISelLowering.cpp | 35 ++++++++++++++++++++++++----------
> > 2 files changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/Target/R600/AMDGPUISelDAGToDAG.cpp b/lib/Target/R600/AMDGPUISelDAGToDAG.cpp
> > index df4461e..bba753f 100644
> > --- a/lib/Target/R600/AMDGPUISelDAGToDAG.cpp
> > +++ b/lib/Target/R600/AMDGPUISelDAGToDAG.cpp
> > @@ -649,9 +649,9 @@ bool AMDGPUDAGToDAGISel::isConstantLoad(const LoadSDNode *N, int CbId) const {
> > }
> >
> > bool AMDGPUDAGToDAGISel::isGlobalLoad(const LoadSDNode *N) const {
> > + //TODO: Why do we need this?
> > if (N->getAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS)
> > - if (Subtarget->getGeneration() < AMDGPUSubtarget::SOUTHERN_ISLANDS ||
> > - N->getMemoryVT().bitsLT(MVT::i32))
> > + if (N->getMemoryVT().bitsLT(MVT::i32))
> > return true;
>
> We shouldn’t. I’ve been wanting to remove the is*Load functions and
> replace them with more normal PatFrags. The memory type checking here
> is also very odd
The question was aimed at the
CONSTANT_ADDRESSS && N->getMemoryVT().bitsLT(MVT::i32) part
bunch of test fail when I remove it, I'll dig further
>
> >
> > return checkType(N->getMemOperand()->getValue(), AMDGPUAS::GLOBAL_ADDRESS);
> > diff --git a/lib/Target/R600/R600ISelLowering.cpp b/lib/Target/R600/R600ISelLowering.cpp
> > index 8357b6d..33909bf 100644
> > --- a/lib/Target/R600/R600ISelLowering.cpp
> > +++ b/lib/Target/R600/R600ISelLowering.cpp
> > @@ -1469,18 +1469,33 @@ SDValue R600TargetLowering::LowerLOAD(SDValue Op, SelectionDAG &DAG) const
> > }
> >
> > // Lower loads constant address space global variable loads
> > - if (LoadNode->getAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS &&
> > - isa<GlobalVariable>(GetUnderlyingObject(
> > + if ((LoadNode->getAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS) &&
> > + (LoadNode->getExtensionType() != ISD::SEXTLOAD)) {
> > + if (isa<GlobalVariable>(GetUnderlyingObject(
> > LoadNode->getMemOperand()->getValue(), *getDataLayout()))) {
> >
> > - SDValue Ptr = DAG.getZExtOrTrunc(LoadNode->getBasePtr(), DL,
> > - getPointerTy(AMDGPUAS::PRIVATE_ADDRESS));
> > - Ptr = DAG.getNode(ISD::SRL, DL, MVT::i32, Ptr,
> > - DAG.getConstant(2, DL, MVT::i32));
> > - return DAG.getNode(AMDGPUISD::REGISTER_LOAD, DL, Op->getVTList(),
> > - LoadNode->getChain(), Ptr,
> > - DAG.getTargetConstant(0, DL, MVT::i32),
> > - Op.getOperand(2));
> > + SDValue Ptr = DAG.getZExtOrTrunc(LoadNode->getBasePtr(), DL,
> > + getPointerTy(AMDGPUAS::PRIVATE_ADDRESS));
> > + Ptr = DAG.getNode(ISD::SRL, DL, MVT::i32, Ptr,
> > + DAG.getConstant(2, DL, MVT::i32));
> > + return DAG.getNode(AMDGPUISD::REGISTER_LOAD, DL, Op->getVTList(),
> > + LoadNode->getChain(), Ptr,
> > + DAG.getTargetConstant(0, DL, MVT::i32),
> > + Op.getOperand(2));
> > + } else {
> > + /* Replace with global load from the same address */
> C style comment.
>
> > + EVT MemVT = LoadNode->getMemoryVT();
> > + Type *InitTy = LoadNode->getMemOperand()->getValue()->getType();
> > + PointerType *PtrTy = PointerType::get(InitTy, AMDGPUAS::GLOBAL_ADDRESS);
> > + Ptr = DAG.getZExtOrTrunc(Ptr, DL, getPointerTy(AMDGPUAS::GLOBAL_ADDRESS));
> > + return DAG.getExtLoad(LoadNode->getExtensionType(),
> > + DL, VT, Chain, Ptr,
> > + MachinePointerInfo(UndefValue::get(PtrTy)), MemVT,
> > + LoadNode->isVolatile(),
> > + LoadNode->isNonTemporal(),
> > + LoadNode->isInvariant(),
> > + LoadNode->getAlignment());
> > + }
> > }
>
> I’m not sure you really need to do this. I would rather not replace the
> underlying address space of the value, since keeping the original
> address space in the MachineMemOperand can be useful information later.
> The MachineMemOperand form of the get*Load functions is also shorter
> and less error prone.
It is still needed, for now. the mesa side[0] uses different vtx buffers
for rodata and user provided constant pointers. The user provided
constant pointers point to Global AS, hence this conversion.
The current status is that the global load instructions are used for
constant loads (for those that did not get lowered into Private AS
reinstantiation).
The only thing this series changes is that instead of Private AS
instantiation it switches to using different VTX buffer for RODATA.
the next step will be to use the same VTX buffer for user const data and
rodata, either by moving rodata to vtx1 or user const data to vtx2.
I plan to continue working on this. the direct benefit of these patches
is that it makes program scope arrays that are big, or use
(r600-)illegal type, work.
Although there is still a minor bug (see patch 2/3 of the mesa series)
jan
[0]http://lists.freedesktop.org/archives/mesa-dev/2015-May/thread.html#85061
>
>
> >
> > if (LoadNode->getAddressSpace() == AMDGPUAS::LOCAL_ADDRESS && VT.isVector()) {
> > --
> > 2.1.0
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
--
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150526/bbafa76d/attachment.sig>
More information about the llvm-commits
mailing list