[PATCH] D81788: [OpenMPOpt] ICV Tracking

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 marked 4 inline comments as done.
sstefan1 added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:837
+    return Changed;
+  }
+
----------------
jdoerfert wrote:
> 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?
I guess we would first need to be aware of 'other' reads and writes, since without that information we could for example remove a setter that was supposed to be read.

I think we can do that with this approach as well. If the value wasn't read when we reach new setter, we mark the last one for removal? Not really sure if one is better than other.


================
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,
----------------
jdoerfert wrote:
> Somehow broken.
The comment is broken?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:889
+        if (!pair.first->comesBefore(I))
+          continue;
+
----------------
jdoerfert wrote:
> Make both conditionals early exits
I guess this makes sense now, but as we improve this, more conditionals will be added and it won't make sense to exit early.


================
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
----------------
jdoerfert wrote:
> I guess this should be `use(10)` too, right? And tmp4 should be removed.
Yes, but only once we add callsite information. Right now we don't know if `use` might have set a new value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81788