[PATCH] D38978: [OpenMP] Enable the lowering of implicitly shared variables in OpenMP GPU-offloaded target regions to the GPU shared memory

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1737
+    if (IsKernelFunction && GenerateSharedDepot) {
+      O << "\t.shared .align " << MFI.getMaxAlignment() << " .b8 \t" << SHARED_DEPOTNAME
+        << getFunctionNumber() << "[" << NumBytes << "];\n";
----------------
Line too long.


================
Comment at: lib/Target/NVPTX/NVPTXFrameLowering.cpp:71
+    // The other device functions need to get a handle on this shared depot
+    // by interacting with the runtime.
+    if (isKernelFunction(*MF.getFunction()) && SharedStackPointerInit) {
----------------
In other places in this patch you refer explicitly to OpenMP, so it probably makes sense to say "the OpenMP runtime" here as well (but just saying "the runtime" seems potentially confusing).


================
Comment at: lib/Target/NVPTX/NVPTXFrameLowering.cpp:85
+               .addReg(NVPTX::VRFrameShared);
+      BuildMI(MBB, MI, dl, MF.getSubtarget().getInstrInfo()->get(MovSharedDepotOpcode),
+              NVPTX::VRFrameShared)
----------------
Line too long.


================
Comment at: lib/Target/NVPTX/NVPTXLowerSharedFrameIndicesPass.cpp:12
+// to remove unneeded functionality and to handle virtual registers. This pass
+// lowers the frame indices to the shared framed index wherever needed.
+//
----------------
Can you be more specific? I believe that we fixed PEI to handle virtual registers, so if that's the only motivation, can we use the regular PEI now?


================
Comment at: lib/Target/NVPTX/NVPTXRegisterInfo.cpp:134
+
+unsigned NVPTXRegisterInfo::getSharedFrameRegister(const MachineFunction &MF) const {
+  return NVPTX::VRShared;
----------------
Line too long.


================
Comment at: lib/Target/NVPTX/NVPTXUtilities.cpp:321
+/// the address of this pointer.
+bool ptrIsStored(Value *Ptr) {
+  SmallVector<const Value*, 16> PointerAliases;
----------------
Can't you use PointerMayBeCaptured (include/llvm/Analysis/CaptureTracking.h) instead of this function? If so, please do.


Repository:
  rL LLVM

https://reviews.llvm.org/D38978