[Libclc-dev] [PATCH 1/1] Implement vload_half{,n}

[Jan Vesely via Libclc-dev]

On Tue, 2017-08-29 at 13:46 +0000, Aaron Watry via Libclc-dev wrote:
> [SNIP]
> > > > 
> > > > I'm not a fan either. for now this patch follows vstore_half for
> > > > consistency. I thinks there is a bigger problem with using LLVM AS in
> > > > libclc. I've been considering 2 ways to address it:
> > > > 
> > > > a) rewrite everything in CLC. This will require adding clang builtins
> > > > for missing functionality.
> > > > 
> > > > b) pass .ll files through clang preprocessor. This would allow .ll
> > > > includes (for target data layout), and target specific defines in
> > > > shared .ll code (like clang/llvm version dependent AS mapping).
> > > > 
> > > > I have a slight preference for a), but it might restrict what
> > > > clang/llvm version we support (or add workarounds like the current
> > > > waitcnt implementation). I'd also have to see how receptive the clang
> > > > ppl are to having builtins for everything that we need and can't be
> > > > done with pure CLC.
> > > > 
> > > > Do you think this should be addressed before any changes that use .ll
> > > > files land?
> > > 
> > > For now, I'd settle for renaming __clc_vload_half_float_helper__* to
> > > just indicate the numeric address space being loaded from (e.g.
> > > __clc_vload_half_float_helper_addr1), and then map those to named
> > > address spaces via the .cl/.inc files in generic/lib/shared/.
> > 
> > This is very awkward solution, because it introduces temporary solution
> > that is broken in a different way from existing vstore code. I'll stall
> >  the patches until a proper solution lands in clang.
> > 
> 
> Up to you.
> 
> 
> > > 
> > > That way, if we have to change them later, there's less confusion
> > > about calling a function named *__global for what is now the
> > > local/whatever address space.
> > 
> > I don't think this is happening, otherwise the tests would fail calling
> > function on wrong type of pointer.
> > 
> > 
> > 
> > 
> 
> This is just me referring to the previous talk about how there are
> tentative plans to renumber the address space IDs in AMDGPU in the future.
> Not something that has happened yet.  I was just trying to minimize the
> number of lines of code change if that happens.
> 
> For now, since we really only support R600/GCN GPUs, we haven't really
> needed to dynamically map address space names to numbers.
> 
> What you've got is ok, just requires more change if that future renumbering
> happens, or if someone starts to care about ptx and libclc.

Guess I misread that. Anyway, it got me to actually start working on a
proper solution. My plan is to use clang builtin [0] to implement
vload/vstore_half. That would handle possible future AS numbering
changes, as well as proper support for nvptx.
*_helpers.ll will be kept around for llvm-4 (and 5, I think it's too
late to get [0] into 5.0). I can check nvptx AS numbering and move
helpers.ll to amdgpu if need be.

regards,
Jan


[0]https://reviews.llvm.org/D37231

> 
> --Aaron
> 
> 
> > regards,
> > Jan
> > 
> > > 
> > > --Aaron
> > > 
> > > > 
> > > > Jan
> > > > 
[SNIP]
-- 
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.llvm.org/pipermail/libclc-dev/attachments/20170829/91440d2b/attachment.sig>