[PATCH] D83723: [OpenMP] Generalize CGOpenMPRuntimeNVPTX as CGOpenMPRuntimeGPU
JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:39 + // return constant compile-time target-specific warp size + unsigned WarpSize = CGF.getTarget().getGridValue(llvm::omp::GV_Warp_Size); + return Bld.getInt32(WarpSize); ABataev wrote: > This is new functionality, better to move it in a separate patch, and this > one mark as NFC. Works for me. This patch shows how the per-target parts are intended to be done. First patch being totally NFC seems good. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83723/new/ https://reviews.llvm.org/D83723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D83723: [OpenMP] Generalize CGOpenMPRuntimeNVPTX as CGOpenMPRuntimeGPU
ABataev added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:39 + // return constant compile-time target-specific warp size + unsigned WarpSize = CGF.getTarget().getGridValue(llvm::omp::GV_Warp_Size); + return Bld.getInt32(WarpSize); This is new functionality, better to move it in a separate patch, and this one mark as NFC. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83723/new/ https://reviews.llvm.org/D83723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D83723: [OpenMP] Generalize CGOpenMPRuntimeNVPTX as CGOpenMPRuntimeGPU
JonChesterfield added a comment. This is indeed the direction I had in mind. Broad strokes look right. I probably wouldn't notice an accidental change amidst the whitespace reshuffle. Very happy to read through line by line if you can split the whitespace change out. Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.h:377 /// Choose a default value for the dist_schedule clause. - void getDefaultDistScheduleAndChunk(CodeGenFunction , - const OMPLoopDirective , OpenMPDistScheduleClauseKind , - llvm::Value *) const override; + void + getDefaultDistScheduleAndChunk(CodeGenFunction , It's worth avoiding whitespace-only changes in a large diff, even when it brings the code in line with clang-format's rules. Signal to noise is challenging enough without it. Please would you leave the whitespace-only changes out? I usually open the diff in a friendly GUI and eyeball each segment to see if it can be dropped. Feel free to fix the whitespace before or after. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83723/new/ https://reviews.llvm.org/D83723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
