[PATCH 1/1] R600: Use TargetConstant in LowerConstantInitializer

[Matt Arsenault]

On 05/21/2014 11:25 AM, Jan Vesely wrote:
>
>
>
> On Wed, May 21, 2014 at 12:57 PM, Matt Arsenault <arsenm2 at gmail.com 
> <mailto:arsenm2 at gmail.com>> wrote:
>
>
>     On May 21, 2014, at 9:43 AM, Matt Arsenault <arsenm2 at gmail.com
>     <mailto:arsenm2 at gmail.com>> wrote:
>
>>
>>     On May 21, 2014, at 9:30 AM, Matt Arsenault <arsenm2 at gmail.com
>>     <mailto:arsenm2 at gmail.com>> wrote:
>>
>>>
>>>     On May 21, 2014, at 2:28 AM, Matt Arsenault <arsenm2 at gmail.com
>>>     <mailto:arsenm2 at gmail.com>> wrote:
>>>
>>>>
>>>>
>>>>     So I think this is sort of correct, but I don't know why this
>>>>     function is doing what it's doing. I'm pretty sure you don't
>>>>     want to use TargetConstant to fix this. It's copying the
>>>>     constant initializer onto the "stack", but expanding it into a
>>>>     legal type so the private memory copy doesn't have the exact
>>>>     same layout as in the constant. I don't know if that's OK or
>>>>     not since I don't know why this is copying it.
>>>>
>>>>     Besides that, I think it would be better to get the type to use
>>>>     with TLI.getRegisterType() rather than using
>>>>     getSmallestLegalIntType. You should also keep the variable as
>>>>     MVT since you're trying to only use legal types here.
>>>>
>>>
>>>     I think the copying is a workaround for not supporting really
>>>     setting up the constant buffer. The constant initializer should
>>>     really be copied into some data section of the binary, and the
>>>     runtime should load the data into a buffer the kernel will read
>>>     from, but that seems to not be implemented.
>
> (based on the original gv-const-addrspace code) the compiler loads 
> literals for every element of global array to private memory and then 
> chooses from there. It explains why I could not get it to work with 
> bigger array than 32 elements.
>
>>
>>     Which would also make this incorrect, since what you really want
>>     is a truncating store to the correct size at the correct address,
>>     but this will store a second i8 in an array 4 bytes later which
>>     is wrong. I'm not sure how much effort there is in fixing this
>>     workaround vs. really loading the constant data.
>>
>>     I noticed a few other problems with this, such as structs and
>>     vectors being broken (which I've similarly partially fixed),
>>     which also are made more difficult by this coming after vector
>>     legalization. These issues would just go away if if the global
>>     address was replaced with an implicit kernel argument that the
>>     runtime loads the data into.
>
> yeah, this patch is turning out not be the quick fix I thought it 
> would, I'd say it's best to drop it for now.
> I think it'd be best to just use vram for global variables (and 
> constant address space), we can reuse code that exists for glbal AS*. 
> Tom mentioned he might have some patches in this regard.
Yes, I would drop it. I'll commit the test case parts


>
>
>     Your test changes are also incorrect. Your check lines now use
>     SI-CHECK/EG-CHECK, but the check prefixes are only SI / EG
>
> I missed this part. I can fix it in v3, but I think it would be best 
> to just drop this patch, and fix the tests when a proper solution for 
> constant AS lands.
>
> regards,
> Jan
>
> * const buff space on EG is too small for OpenCL FULL PROFILE anyway. 
> OTOH, limiting this hw to EMBEDDED_PROFILE, so it can use const buffer 
> for const as, would enable us to hide the rather slow int64 support 
> behind an extension.
>
Constant initializers should be fine to handle in the constant buffers 
since if it doesn't fit you can fall back to global, and the common case 
is a small initializer. For constant pointer arguments, you have to use 
global and there is an AMD extension max_constant_size attribute you can 
place on constant pointers to allow assuming they are small enough to fit
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140521/f5663b88/attachment.html>