[PATCH] D94814: [HIP] Support `__managed__` attribute

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

Presumably, `__managed__` variables would have to be memory-mapped into the host address space.



================
Comment at: clang/lib/CodeGen/CGCUDANV.cpp:391
+  SmallVector<SmallVector<llvm::User *, 8>, 8> WorkList;
+  for (auto &&UU : Var->uses()) {
+    WorkList.push_back({UU.getUser()});
----------------
`VarUse` ?


================
Comment at: clang/lib/CodeGen/CGCUDANV.cpp:395
+  while (!WorkList.empty()) {
+    auto &&US = WorkList.pop_back_val();
+    auto *U = US.back();
----------------
`WorkItem`


================
Comment at: clang/lib/CodeGen/CGCUDANV.cpp:552-553
+            llvm::ConstantInt::get(IntTy, Info.Flags.isConstant()),
+            llvm::ConstantInt::get(IntTy, CGM.getLangOpts().HIP &&
+                                              Info.Flags.isManaged())};
+        Builder.CreateCall(RegisterVar, Args);
----------------
This will always be `false`, because all cases where `Info.Flags.isManaged()` is true are handled in the other branch of this `if`.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7728
+  case ParsedAttr::AT_HIPManaged:
+    handleSimpleAttributeWithExclusions<HIPManagedAttr, CUDAGlobalAttr>(S, D,
+                                                                        AL);
----------------
The code changes in the patch appear to treat `__managed__` variable as a superset of a `__device__` var. If that's indeed the case, adding an implicit `__device__` attribute here would help to simplify the code. This way the existing code can handle generic  `__device__` var functionality without additional changes, and would use `__managed__` checks for the cases specific for managed vars only.



================
Comment at: llvm/lib/IR/ReplaceConstant.cpp:20
+namespace llvm {
+Instruction *createReplacementInstr(ConstantExpr *CE, Instruction *Instr) {
+  IRBuilder<NoFolder> Builder(Instr);
----------------
The function could use a descriptive comment.

Despite the comment above, it does seem to do any replacing itself. Presumably it creates a an instruction that would perform an equivalent calculation at runtime.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94814/new/

https://reviews.llvm.org/D94814