[Openmp-commits] [PATCH] D139287: [OpenMP] Introduce basic JIT support to OpenMP target offloading

[Joseph Huber via Phabricator via Openmp-commits]

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