jdoerfert added a comment.
I think this is now style wise pretty good, I still found some potential
problems below.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1487
+ // possible, or else at the end of the function.
+ void emitBlock(BasicBlock *BB, Function *CurFn, bool IsFinished = false);
+
----------------
jdoerfert wrote:
> This does not mention the deletion stuff, etc.
This talks about creating a new block, but reading the function it seems it
will just place `\p BB`. Which one is it?
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4122
+ Builder.CreateCall(getOrCreateRuntimeFunctionPtr(*MapperFunc),
+ OffloadingArgs);
+ } else {
----------------
Can we at least assert MapperFunc is non null if standalone is true. It seems
like an invariant almost worth documenting, at least with an assertion +
message.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4761
+ if (CondConstant)
+ ThenGen(Builder.saveIP(), Builder.saveIP());
+ else
----------------
In the call above you pass the AllocaIP here, now it is both times the saveIP.
I doubt this is correct. Allocas will end up in the wrong spot, won't they? Do
we have a test for that?
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4741
+ if (IsFinished && BB->use_empty()) {
+ delete BB;
+ return;
----------------
TIFitis wrote:
> jdoerfert wrote:
> > you should not just delete BB. eraseFromParent is a better way. That said,
> > it is also weird you would delete something in an "emit" function.
> I've changed it.
>
> The `emitBlock`, `emitBranch` and `emitIfClause` functions are ported from
> Clang btw.
> //clang/lib/CodeGen/CGStmt.cpp//
That might be so, but given the name and initial documentation, nobody would
have expected this function to delete the block, or at least I would not have.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146557/new/
https://reviews.llvm.org/D146557
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
