R600/SI Patchset: Initial support for compute shaders

[Tom Stellard]

On Mon, Mar 11, 2013 at 05:24:36PM +0100, Christian König wrote:
> Am 11.03.2013 14:59, schrieb Tom Stellard:
> >On Mon, Mar 11, 2013 at 10:03:53AM +0100, Christian K?nig wrote:
> >>Am 08.03.2013 22:29, schrieb Tom Stellard:
> >>>Hi,
> >>>
> >>>The attached patches add support for the BUFFER_STORE instruction and
> >>>make it possible to run simple compute shaders on Southern Islands
> >>>hardware with the open source radeonsi GPU driver in
> >>>Mesa (http://www.mesa3d.org).
> >>>
> >>>-Tom
> >>>
> >>>0001-R600-SI-Avoid-generating-S_MOVs-with-64-bit-immediat.patch
> >>>
> >>>
> >>>>From aa3e075e2f0df66e2fe50018704aee28904155f0 Mon Sep 17 00:00:00 2001
> >>>From: Tom Stellard<thomas.stellard at amd.com>
> >>>Date: Wed, 6 Mar 2013 17:28:55 +0000
> >>>Subject: [PATCH 1/5] R600/SI: Avoid generating S_MOVs with 64-bit immediates
> >>>
> >>>SITargetLowering::analyzeImmediate() was converting the 64-bit values
> >>>to 32-bit and then checking if they were an inline immediate.  Some
> >>>of these conversions caused this check to succeed and produced
> >>>S_MOV instructions with 64-bit immediates, which are illegal.
> >>>---
> >>>   lib/Target/R600/SIISelLowering.cpp |   11 +++++++++--
> >>>   test/CodeGen/R600/imm.ll           |   34 ++++++++++++++++++++++++++++++++++
> >>>   2 files changed, 43 insertions(+), 2 deletions(-)
> >>>   create mode 100644 test/CodeGen/R600/imm.ll
> >>>
> >>>diff --git a/lib/Target/R600/SIISelLowering.cpp b/lib/Target/R600/SIISelLowering.cpp
> >>>index fead115..74b1dc5 100644
> >>>--- a/lib/Target/R600/SIISelLowering.cpp
> >>>+++ b/lib/Target/R600/SIISelLowering.cpp
> >>>@@ -426,9 +426,16 @@ int32_t SITargetLowering::analyzeImmediate(const SDNode *N) const {
> >>>       float F;
> >>>     } Imm;
> >>>-  if (const ConstantSDNode *Node = dyn_cast<ConstantSDNode>(N))
> >>>+  if (const ConstantSDNode *Node = dyn_cast<ConstantSDNode>(N)) {
> >>>+    if (Node->getValueType(0) == MVT::i64) {
> >>>+      int64_t Imm64 = Node->getZExtValue();
> >>>+      if (Imm64 >> 32) {
> >>>+        return -1;
> >>>+      }
> >>>+      Imm.I = Imm64;
> >>>+    }
> >>>       Imm.I = Node->getSExtValue();
> >>I wouldn't write it like this, setting Imm.I twice is a bit superfluous.
> >>Also I don't think we should look at the ValueType, maybe something like
> >>this should be more appropriate:
> >>
> >>if (const ConstantSDNode *Node = dyn_cast<ConstantSDNode>(N)) {
> >>    int64_t Imm64 = Node->getSExtValue();
> >>    if (Imm64 >> 32)
> >>      return -1;
> >>    Imm.I = Imm64;
> >>} else...
> >>
> >This make sense, but what if this function were used on a 32-bit
> >immediate?  Wouldn't if(Imm64 >> 32) be true for all negative integers?
> >If so, then we would miss some opportunities for using inline
> >immediates.
> 
> Ups, yeah your right. But checking for i64 type isn't 100% correct
> either, cause the inline immediates are definately sign extended.
>

I just realized that your solution will work fine.  We can't use a single
inline immediate for 64-bit values anyway because we need the sign to
extend into the high bits, so any negative 64-bit immediate needs to
be rejected.

-Tom