Pierre-vh wrote: > The change introduces new builtins and intrinsics, implements them > end-to-end, and documents their semantics. It's a complete self-contained > patch.
At a glance it looks like asyncmark/asyncwait could have been a separate patch. > I don't see what value is added by splitting this into smaller patches, just > for the sake of smallness. It's easier to review, even if everything lands all at once. e.g. in this patch, there's ~2k lines, and in my first read through I missed the entirety of the `InsertWaitCnt` changes because GitHub hid it due to size, and it was sandwiched between two larger files. In any case, I don't want to side track the review too much. It's fine for this review, this is a big patch, but it's no so big that splitting it is a blocker for review in my book. Just keep it in mind next time and try to split things up a bit more when there is an opportunity to do so. It really helps with reviews :smile: > What's missing from the overview presented by the patch description? The new builtins like `__builtin_amdgcn_raw_ptr_buffer_load_async_lds` https://github.com/llvm/llvm-project/pull/173259 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
