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

[Alexey Bader via Phabricator via llvm-commits]

bader marked 2 inline comments as done.
bader added inline comments.


================
Comment at: clang/include/clang/AST/Type.h:493
+           // Default is a superset of SYCL address spaces.
+           (A == LangAS::Default &&
+            (B == LangAS::sycl_private || B == LangAS::sycl_local ||
----------------
Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > Ok if you allow implicit conversions both ways then this condition should be extended to also contain all named address spaces in `A` and `Default` in `B`. But actually, could you simplify by checking that you have `Default` on either side, so something like 
> > > > > 
> > > > > 
> > > > > ```
> > > > > (A == LangAS::Default || B == LangAS::Default)
> > > > > ```
> > > > > ?
> > > > > Ok if you allow implicit conversions both ways then this condition should be extended to also contain all named address spaces in `A` and `Default` in `B`. But actually, could you simplify by checking that you have `Default` on either side, so something like 
> > > > > 
> > > > > 
> > > > > ```
> > > > > (A == LangAS::Default || B == LangAS::Default)
> > > > > ```
> > > > > ?
> > > > 
> > > > According to the comment above `isAddressSpaceSupersetOf` function definition.
> > > > ```
> > > >   /// Returns true if address space A is equal to or a superset of B.
> > > > ```
> > > > 
> > > > `(A == LangAS::Default || B == LangAS::Default)` <- this change makes `Default` address space a superset of all address spaces including OpenCL, which we were trying to avoid with adding SYCL address spaces. Another problem with this code is that make `Default` a **sub-set** of named address spaces (like `sycl_local`), which is not right.
> > > > If I understand it correctly defining "isSupersSetOf" relation is enough for the rest of framework to enable conversions. Am I right?
> > > > (A == LangAS::Default || B == LangAS::Default) <- this change makes Default address space a superset of all address spaces including OpenCL.
> > > 
> > > I see, yes this will break pretty much everything unless we guard by SYCL mode. But I don't think it is good to go this route though.
> > > 
> > > > Another problem with this code is that make Default a sub-set of named address spaces (like sycl_local), which is not right.
> > > 
> > > Well, if you need implicit conversions to work both ways as you have written in the documentation then you don't really have a true super-/subsets between the named address spaces and the default one. They appear to be equivalent.
> > > 
> > > ```
> > > SYCL mode enables both explicit and implicit conversion to/from the default address space from/to
> > > the address space-attributed type.
> > > ```
> > > 
> > > So do you actually need something like this to work?
> > > 
> > > ```
> > > int * genptr = ...;
> > > __private int * privptr = genptr:
> > > ```
> > > 
> > > 
> > I looked though the code base and I see that explicit cast is used when raw pointer is casted to address space annotated type. I think we can always use explicit cast from `Default` to named address space instead of implicit cast. It might be even useful to avoid unintended implicit casts causing UB.
> > @keryell, @Naghasan, what do you think if we update https://reviews.llvm.org/D99488 to disallow implicit casts from `Default` to named address space? I think it should be okay considering that current implementation doesn't use this type of casts (and I can't come up with a use case for it).
> > 
> > Meanwhile I've added checks for that to clang/test/SemaSYCL/address-space-conversions.cpp.
> Do you still plan to wait for extra input or otherwise we could just update the documentation for now?
> 
> If you discover that you need to allow the reverse conversions later it should not be problematic to add since it won't break anyone's code. It will only allow more code to compile!
Okay. I'll update the document right away.


================
Comment at: clang/lib/Basic/Targets/SPIR.h:140
+    // space must be compatible with the generic address space
+    return LangAS::sycl_global;
+  }
----------------
Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > This needs a language guard too?
> > I can re-write this method to avoid using language address space:
> > 
> > ```
> >   llvm::Optional<LangAS> getConstantAddressSpace() const override {
> >     return getLangASFromTargetAS(1);
> >   }
> > ```
> > 
> > Does it look okay to you?
> > 
> > I don't think we need a language guard here as this hook is already guarded by users. E.g. https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenModule.cpp#L4137-L4159.
> > Adding language guards for `TargetInfo::getConstantAddressSpace` method will require API change similar to `adjust` method i.e. explicit `LangOptions` type parameter.
> Well, OpenCL is not the only language mode though so it generally is an ABI change. But also there are seem to be other uses of this function, for example in `CGExpr.cpp` that are not guarded by the language mode and there it seems like constant address space might make more sense for OpenCL. 
> 
> I think the situation is similar here as for global variables - we might need a constant alternative to  `CodeGenModule::GetGlobalVarAddressSpace`. But for the time being if you only need this logic for the purposes of `castStringLiteralToDefaultAddressSpace` why not to just add a SYCL special case straight there? I don't think your current implementation would allow using any other address space for this anyway?
This logic should be applied to all types of literals, so changing only `castStringLiteralToDefaultAddressSpace` is not enough. I think this is where the most of the CodeGen changes from the first version of the patch are coming from (if not all of them). I'll check if it can be refactored to something like `CodeGenModule::GetGlobalVarAddressSpace`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909