[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via llvm-commits]

bader added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:4596
+                                            IRFuncTy->getParamType(FirstIRArg));
+        }
+
----------------
bader wrote:
> bader wrote:
> > rjmccall wrote:
> > > This seems problematic; code like this shouldn't be necessary in every place that uses a pointer value.  The general rule needs to be that expression emission produces a value of its expected type, so the places that emit pointers need to be casting to the generic address space.  We can avoid that in some special expression emitters, like maybe EmitLValue, as long as the LValue records what happened and can apply the appropriate transform later.
> > > 
> > > Also, in IRGen we work as much as possible with AST address spaces rather than IR address spaces.  That is, code like this that's checking for address space mismatches needs to be comparing the source AST address space with the destination AST address space and calling a method on the TargetInfo when a mismatch is detected, rather than comparing IR address spaces and directly building an `addrspacecast`.  The idea is that in principle a target should be able to have two logical address spaces that are actually implemented in IR with the same underlying address space, with some arbitrary transform between them.
> > @erichkeane, @asavonic, could you help with addressing this concern, please?
> I looked into this and managed to replace almost all CodeGen library changes with a couple of hooks for SPIR target. The one change, which is still required is located in clang/lib/CodeGen/CGDecl.cpp and I think the assertion fixed by this change can be reproduced by compiling C++ for AMDGPU target. The assertion happens due to invalid bitcast in constant expression and it's triggered by one of the cases in clang/test/CodeGenSYCL/address-spaces.cpp:
> 
> ```
>   const char *str = "Hello, world!";
>   i = str[0];
>   const char *phi_str = i > 2 ? str : "Another hello world!";
>   const char *select_null = i > 2 ? "Yet another Hello world" : nullptr;
>   const char *select_str_trivial1 = true ? str : "Another hello world!";
> 
> ```
> I also see that using this approach we get much more addrspacecast instructions. I hope it won't be problem, I'm going to apply the same change to our fork and run additional tests to make sure there no other problems with that patch.
> 
> @rjmccall, thanks a lot for valuable feedback!
I was wrong about the test case triggering the bug, but I managed to reproduce it for amdgpu target from a C source. I created a separate review request for this particular fix - https://reviews.llvm.org/D92782 to untie it from the attributes part of this patch.

While I was integrating this approach to our SYCL implementation I found another CodeGen bug, which I'll send separately.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909