Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.
Thanks for helping to complete the OpenMPIRBuilder implementation!
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2586-2587
void CodeGenFunction::EmitOMPSimdDirective(const OMPSimdDirective &S) {
+
+ bool UseOMPIRBuilder = CGM.getLangOpts().OpenMPIRBuilder;
+ if (UseOMPIRBuilder) {
----------------
[nit] Unnecessary whitespace
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2587
+
+ bool UseOMPIRBuilder = CGM.getLangOpts().OpenMPIRBuilder;
+ if (UseOMPIRBuilder) {
----------------
Did you consider adding a check whether OpenMPIRBuilder actually implements all
features requested before using it? See `isSupportedByOpenMPIRBuilder`.
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2594
+ // Emit the associated statement and get its loop representation.
+ auto DL = SourceLocToDebugLoc(S.getBeginLoc());
+ const Stmt *Inner = S.getRawStmt();
----------------
[style] According to the [[
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
| LLVM coding standard ]], `auto` is only used in specific cases.
================
Comment at: clang/test/OpenMP/simd_codegen_irbuilder.cpp:16
+ // CHECK-NEXT: call void @__captured_stmt.1(i32* %i, i32 %omp_loop.iv,
%struct.anon.0* %agg.captured1)
+ // CHECK-NEXT: %3 = load float*, float** %b.addr, align 8,
!llvm.access.group !3
+ // CHECK-NEXT: %4 = load i32, i32* %i, align 4, !llvm.access.group !3
----------------
Could you use regexes to match the virtual register names? The script
`update_cc_checks.py` may help with this, see the other tests.
The naming scheme for files testing the OpenMPIRBiulder is usually
`irbuilder_<constructs>`
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:524
+ /// \param Loop The loop to simd-ise.
+ void createSIMDLoop(DebugLoc DL, CanonicalLoopInfo *Loop);
+
----------------
Capitalizing SIMD leads to long runs of capital letters. While not an LLVM
coding style rule, it is still common to title-case acronyms in identifiers if
the acronym is ~4 letters or more, such as `OMPSimdDirective` instead of
`OMPSIMDDirective`.
For methods transformation `CanonicalLoopInfo`, we don't use `createXYZ` naming
scheme which is reserved for inserting something new. Suggestions: `applySimd`,
`vectorizeLoop`, or `simdizeLoop`.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2120
+/// Attach metadata access.group to the load and store instructions of \p block
+static void addSIMDMetadata(BasicBlock *block,
+ ArrayRef<Metadata *> Properties) {
----------------
The LLVM coding style still capitalizes parameters and variables.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2123
+ for (auto &I : *block) {
+ if (isa<LoadInst>(&I) || isa<StoreInst>(&I)) {
+ Instruction *instr = dyn_cast<Instruction>(&I);
----------------
The property also applies to any memory access, e.g. `memset`. You could use
`mayWriteToMemory()` and `mayReadFromMemory()`.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2126
+ LLVMContext &C = instr->getContext();
+ MDNode *N = MDNode::get(C, MDString::get(C, ""));
+ instr->setMetadata("llvm.access.group", N);
----------------
[serious] The access group must be unique only to the loop being accessed.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2157-2168
+ addSIMDMetadata(header,
+ {
+ MDNode::get(Ctx, MDString::get(Ctx,
"llvm.access.group")),
+ });
+ addSIMDMetadata(cond,
+ {
+ MDNode::get(Ctx, MDString::get(Ctx,
"llvm.access.group")),
----------------
[serious] header and cond don't contain any memory accesses. `body` may consist
of multiple basic blocks, of which this only applies to the first one.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114379/new/
https://reviews.llvm.org/D114379
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
