[PATCH] D85052: [OpenMPOpt] ICV Tracking Between BasicBlocks

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Follow up with the interprocedural replacement sounds good.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1250
+    return ICVValuesMap[ICV].empty();
+  }
+
----------------
!empty()?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1282
+      auto MapReplacementValues = [&](Use &U, Function &) {
+        Instruction *UserI = cast<Instruction>(U.getUser());
+        Value *ReplVal = getReplacementValue(ICV, UserI, A);
----------------
Don't we want
` CallInst *CI = OpenMPOpt::getCallIfRegularCall(U);`
and a check for CI?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1286
+                .insert(std::make_pair(UserI, ReplVal))
+                .second)
+          HasChanged = ChangeStatus::CHANGED;
----------------
can we have an assert to verify that the getter is either not mapped or mapped to ReplVal. If that triggers it would be bad since this is not monotonic anymore and could just spin.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1324
+    if (ICVTrackingAA.isAssumedTracked())
+      return ICVTrackingAA.hasTrackedValue(ICV);
+
----------------
Does this work if the value was not "tracked", so no setter was seen, but "maybe written", by another unkown call? 

I guess we need a test where we don't call `use1` or `use` but something that then calls `use`.


================
Comment at: llvm/test/Transforms/OpenMP/icv_tracking.ll:19
+  ret i32 %2
+}
+
----------------
sstefan1 wrote:
> jdoerfert wrote:
> > Maybe rename them to indicate if it is a `unknown_callee` or a `benign_callee`.
> I think that name is more appropriate for `@use()` since we can't know if a declaration changes an ICV, right?
> 
> `@use1()` is not a great name, I agree with that. I'll change it.
I meant the first is for `use` the second for `use1`. Having `use` is not too bad as I would assume the worst about it, but `use1` really doesn't capture it is pure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85052