[PATCH] D114940: [OMPIRBuilder] Support ordered clause specified without parameter
Peixin Qiao via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 31 18:19:06 PDT 2022
peixin marked an inline comment as done.
peixin added a comment.
Addressed comments.
================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2228
+ for (auto &EI : *Preheader) {
+ Instruction *cur = &EI;
+ if (isa<CallInst>(cur)) {
----------------
Meinersbur wrote:
> peixin wrote:
> > Meinersbur wrote:
> > > [style] LLVM coding style starts local variables with a capital letter. But preferable, just use `EI.member` instead of `cur->member`.
> > There are a lot of code with this code style issue in OpenMPIRBuilderTest.cpp. Should I create one NFC patch to fix it in case it guide followers using the wrong style?
> Would be nice to do so.
>
> For this patch, I am only looking for the coding style of new code.
OK. Will do it after this patch.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:878-879
+ // nonmonotonic modifier is specified.
+ if (!(schedType == llvm::omp::OMPScheduleType::OrderedStatic ||
+ schedType == llvm::omp::OMPScheduleType::OrderedStaticChunked))
+ schedType |= llvm::omp::OMPScheduleType::ModifierNonmonotonic;
----------------
Meinersbur wrote:
> peixin wrote:
> > Meinersbur wrote:
> > > This doesn't seem right. It says "if the ordered clause is specified", but this does not consider the `orderedVal`. What sets the `Monotonic` modifier?
> > There is also one your question I forgot to answer.
> > > Is the ordered incompatible with static schedules?
> >
> > For these two questions, I think this is the history issue of clang and openmp runtime library. The `Monotonic` is used by default in openmp runtime library if it is not set here. The ordered is compatible with static schedules. But this code is following what clang does.
> Please mention this in a comment
Added in comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114940/new/
https://reviews.llvm.org/D114940
More information about the llvm-commits
mailing list