R600/SI Patchset: Initial support for compute shaders

[Christian König]

Am 12.03.2013 20:08, schrieb 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.

No that's not correct, the point is that inline immediates ARE 64-bit 
sign extended. At least -1 is, cause we use it as initializer for 64-bit 
SGPR "all bits set" on a couple of different occasions (don't know if we 
ever tested the others, but I don't think the behavior would be different).

How about something like this:

if (const ConstantSDNode *Node = dyn_cast<ConstantSDNode>(N)) {
    if (Node->getZExtValue() >> 32)
      return -1;
    Imm.I = Node->getSExtValue();
} else...

(Note the difference between ZExt for the test and SExt for the value).

If I'm not completely wrong that should do it.

Christian.