Pierre-vh wrote: > Sema does need to check the address_space attribute. So maybe it should be > aware that this type has a specific address space which cannot be changed? I > am not really familiar about the division of concerns to be used here, but > from first principles, it feels like the address space of a barrier on AMDGPU > is known as soon as its type is known, which means it should be known to Sema.
The thing I'm not following is that this was not done before, when the emitted AS was __shared__. Why does this patch need to suddenly fix it when it's not the goal/topic of this patch ? This feature is not in an ideal position, my patch stack tries to move the needle a bit, but it's not supposed to fix everything that was missed on the first pass. I do strongly agree this needs more UX improvements (diagnostics, check how it behaves with constexpr, etc.) but it's orthogonal to this patch. I wouldn't mind looking into it as a separate issue (and prioritize it if it's a strong concern). Personally I think the even more pressing issue is that this type does not have any attached documentation, particularly on its object model (which is non-intuitive as this isn't an object/allocation in the traditional sense). If there are precise, directly actionable concerns that directly relate to what this patch is doing I'll be happy to fix them to get the review moving forward. The way I see this patch is more like narrowing down the IR representation of the barrier object from being anywhere in LDS, to being in a specific AS that's built on top of LDS. https://github.com/llvm/llvm-project/pull/195612 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
