jhuber6 added a comment.
Some nits.
================
Comment at:
openmp/libomptarget/plugins-nextgen/common/PluginInterface/JIT.cpp:132
+
+OptimizationLevel getptLevel(unsigned OptLevel) {
+ switch (OptLevel) {
----------------
typo
================
Comment at:
openmp/libomptarget/plugins-nextgen/common/PluginInterface/JIT.cpp:121
+
+CodeGenOpt::Level getCGOptLevel(unsigned OptLevel) {
+ switch (OptLevel) {
----------------
I'm tempted to move this into LLVM somewhere since it's been duplicated so many
times.
================
Comment at:
openmp/libomptarget/plugins-nextgen/common/PluginInterface/JIT.cpp:150
+Expected<std::unique_ptr<TargetMachine>>
+createTargetMachine(Module &M, Triple TT, std::string CPU, unsigned OptLevel) {
+ CodeGenOpt::Level CGOptLevel = getCGOptLevel(OptLevel);
----------------
Why do we need `TT` if we expect to read the triple out of the Module?
================
Comment at:
openmp/libomptarget/plugins-nextgen/common/PluginInterface/JIT.cpp:276-277
+
+ StringRef RawData(CGOutputBuffer.begin(), CGOutputBuffer.size());
+ return MemoryBuffer::getMemBufferCopy(RawData);
+}
----------------
Should work.
================
Comment at:
openmp/libomptarget/plugins-nextgen/common/PluginInterface/JIT.cpp:301
+/// Output images generated from LLVM backend.
+std::list<std::unique_ptr<MemoryBuffer>> JITImages;
+
----------------
Why a `std::list`? Since we use pointers we shouldn't need to worry about
having a stable pointer and could use a `SmallVector` or similar right?
================
Comment at:
openmp/libomptarget/plugins-nextgen/common/PluginInterface/JIT.cpp:319
+
+ StringRef Data((const char *)Image->ImageStart,
+ (char *)Image->ImageEnd - (char *)Image->ImageStart);
----------------
Should probably prefer C++ casts even if they are ridiculously verbose.
================
Comment at:
openmp/libomptarget/plugins-nextgen/common/PluginInterface/JIT.cpp:343
+Expected<__tgt_device_image *> compile(__tgt_device_image *Image,
+ Triple::ArchType TA, std::string MCpu,
+ unsigned OptLevel,
----------------
`MCPU` is the more common version AFAICT.
================
Comment at: openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp:806
+
+ ///
+ struct ComputeCapabilityTy {
----------------
Missing comment?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139287/new/
https://reviews.llvm.org/D139287
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
