tra marked an inline comment as done. tra added inline comments.
================ Comment at: clang/test/CodeGen/builtins-nvptx-mma.py:84 + # It uses __mma_tf32_m16n16k8_ld_c but __mma_m16n16k8_st_c_f32. + make_ldst_ops(["m16n16k8"], ["a", "b", "c", "d"], ["tf32", "f32"])) ---------------- steffenlarsen wrote: > tra wrote: > > steffenlarsen wrote: > > > The following changes would remove the need for the `m16n16k8` cases in > > > `is_ldst_variant_supported`. > > This was done deliberately. I did have that in an earlier variant of the > > patch, but eventually settled on the current version. > > > > My thinking is that `make_ldst_ops(m16n16k8)` would create whatever is > > necessary for `m16n16k8`, and we'd keep the decision of what to produce in > > one place in `is_ldst_variant_supported`. Splitting one make_ldst_ops into > > two here makes this decision implicit which would need additional > > explanations. Code that needs less explanations wins. :-) > > > My concern is that it is more confusing this way because they are the only > load/store ops generated where the type is then later filtered. It makes > sense for MMA because it needs to filter out select combinations, but here > the comment above says one thing and `make_ldst_ops` does something else. > > I don't think it makes a huge difference either way, so if you prefer the > current version I am okay with keeping it. That said, it might be good to > have a comment in the `m16n16k8` case of `is_ldst_variant_supported` making > the connection to this. Fair enough. I've adopted your suggestion. Builtins don't have as much magic as intrinsics, so your approach is indeed simpler. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105384/new/ https://reviews.llvm.org/D105384 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
