bader added inline comments.
================ Comment at: clang/lib/CodeGen/CGCall.cpp:4596 + IRFuncTy->getParamType(FirstIRArg)); + } + ---------------- 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! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89909/new/ https://reviews.llvm.org/D89909 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
