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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
