[PATCH] D99024: [OpenMP][OMPIRBuilder] Adding support for `omp atomic`
Michael Kruse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 31 12:46:09 PDT 2021
Meinersbur added a comment.
You can avoid the "clang-format not found in user's PATH; not linting file." warning using `arc diff --nolint`. It asks for a justification, it accepts every non-empty string, including a single space. Please consider fixing the other warnings.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2124-2128
+ /// CurBB
+ /// | /---\
+ /// ContBB |
+ /// | \---/
+ /// ExitBB
----------------
Triple slashes are doxygen comments, which come before the function. If not a doxygen documentation, please use standard `//` comments.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2129
+ /// ExitBB
+ auto *CurBB = Builder.GetInsertBlock();
+ auto *CurBBTI = CurBB->getTerminator();
----------------
[style] No almost-always-auto in LLVM style
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2161-2164
+ if (Builder.GetInsertPoint() == ContBB->end())
+ llvm::errs() << "end block\n";
+ else
+ ContBB->dump();
----------------
Debugging leftover?
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1922
+bool OpenMPIRBuilder::CheckAndEmitFlushAfterAtomic(
+ const LocationDescription &Loc, llvm::AtomicOrdering AO, AtomicKind AK) {
----------------
Consider making such helpers static functions instead of priviate methods, so they don't need to appear in the public header file.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1936
+
+ if (AK == Read) {
+ if (AO == AtomicOrdering::Acquire || AO == AtomicOrdering::AcquireRelease ||
----------------
fghanim wrote:
> Meinersbur wrote:
> > This if-chain could be done more elegantly with a switch.
> Either:
> 1- do nested if-else
> 2- nested switch case
> 3- a mix of both.
>
> 4- use the magic of logical shifts & bit-wise and/or.s to do this, and get rid of all the messy cfg
>
> switched from 1 to 3, more than happy to do 4
It is fine as-is, just the `Capture` case might justify a nested switch.
Not sure whether approach 4 would make it any better.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2082-2088
+ case AtomicRMWInst::Xchg:
+ case AtomicRMWInst::FAdd:
+ case AtomicRMWInst::FSub:
+ case AtomicRMWInst::BAD_BINOP:
+ case AtomicRMWInst::Max:
+ case AtomicRMWInst::Min:
+ default:
----------------
fghanim wrote:
> Meinersbur wrote:
> > With a default case, no need to enumerate all cases.
>
> I intentionally listed all cases. If you want, i'll remove `default` which I added to future proof the code.
With a default case, the compiler does not warn about unhandled cases anymore. If you want both, you could do:
```
[...]
case AtomicRMWInst::Min:
llvm_unreachable("Unsupported atomic update operation");
}
llvm_unreacheable("Unhandled atomic update operation");
```
Personally I think it just adds clutter lines. In case of some parts of clang's OpenMP implementation it fills entire screens of unsupported cases and there is no chance I miss handling the case during testing due to the llvm_unreachable.
Currently, `return nullptr;` is unreachable code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99024/new/
https://reviews.llvm.org/D99024
More information about the llvm-commits
mailing list