[PATCH] D81788: [OpenMPOpt] ICV Tracking

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.
Herald added a reviewer: baziotis.

Nit: Add documentation to functions and classes please. Describe what happens and why if applicable.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:759
+
+      A.getOrCreateAAFor<AAICVTracker>(IRPosition::function(*F));
+    }
----------------
I'm really happy one can add a local AA like this :)




================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:811
+    auto ICVInfo = OMPInfoCache.ICVs[ICV];
+    auto Getter = OMPInfoCache.RFIs[ICVInfo.Getter];
+
----------------
Style: `&` in both above.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:816
+    if (!UV || UV->size() == 0)
+      return false;
+
----------------
Nit: Is this necessary?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:835
+
+    Getter.foreachUse(ReplaceAndDeleteCB);
+    return Changed;
----------------
Nit: `GetterRFI`


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:837
+    return Changed;
+  }
+
----------------
We might want to "push" the information from setters instead. That would allow us to remove setters that are not useful, e.g. it is reset before it could have been read. We also need to look at other/implicit reads and writes later: https://godbolt.org/z/fdBqiF

I'm fine with starting here but let me know what you think about pushing instead?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:842
+                  InternalControlVar, InternalControlVar::ICV___last>
+      ICVValuesMap;
+
----------------
Can we replace the pair with a struct that has proper member names and documentation.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:877
+
+  /// Return the value with which \p can be replaced.
+  Value *getReplacementValue(InternalControlVar ICV, const Instruction *I,
----------------
Somehow broken.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:885
+    auto &OMPInfoCache = static_cast<OMPInformationCache &>(A.getInfoCache());
+    auto Getter = OMPInfoCache.RFIs[OMPInfoCache.ICVs[ICV].Getter];
+    for (auto &pair : ValuesSet) {
----------------
Style: `&` and `GetterRFI`. 


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:889
+        if (!pair.first->comesBefore(I))
+          continue;
+
----------------
Make both conditionals early exits


================
Comment at: llvm/test/Transforms/OpenMP/icv_tracking.ll:57
   tail call void @use(i32 %4)
 ; FIXME: this value ( min(%3, 10) ) should be tracked and the rest of the getters deduplicated and replaced with it.
   tail call void @omp_set_num_threads(i32 10)
----------------
I thought this min was necessary but I doubt it reading the standard again. Maybe just remove the fixme


================
Comment at: llvm/test/Transforms/OpenMP/icv_tracking.ll:96
 ; CHECK-NEXT:    [[TMP4:%.*]] = tail call i32 @omp_get_max_threads()
 ; CHECK-NEXT:    tail call void @use(i32 [[TMP4]])
 ; CHECK-NEXT:    ret void
----------------
I guess this should be `use(10)` too, right? And tmp4 should be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81788