[PATCH] D81788: [OpenMPOpt] ICV Tracking

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 26 13:41:23 PDT 2020


jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:837
+    return Changed;
+  }
+
----------------
sstefan1 wrote:
> 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.
Both directions conceptually work. Let's wait and see. The correctness should never be compromised, I mean if you see something that you don't know you have to assume it reads and writes the ICV anyway. 


================
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,
----------------
sstefan1 wrote:
> jdoerfert wrote:
> > Somehow broken.
> The comment is broken?
Yes, a stray `\p` , right?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:889
+        if (!pair.first->comesBefore(I))
+          continue;
+
----------------
sstefan1 wrote:
> 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.
It depends. Having 3 early exists is, for me at least, nicer than one big condition.


================
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
----------------
sstefan1 wrote:
> 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.
I see.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81788





More information about the llvm-commits mailing list