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

[Jan Vesely]

On Wed, May 21, 2014 at 12:57 PM, Matt Arsenault <arsenm2 at gmail.com> wrote:

>
> On May 21, 2014, at 9:43 AM, Matt Arsenault <arsenm2 at gmail.com> wrote:
>
>
> On May 21, 2014, at 9:30 AM, Matt Arsenault <arsenm2 at gmail.com> wrote:
>
>
> On May 21, 2014, at 2:28 AM, Matt Arsenault <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.


>
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140521/67a33a3a/attachment.html>