[PATCH] D75893: [OpenMP] Add !range metadata to loads from omp.(ub/lb)

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Sorry for the delay. I'm usually faster.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:249
+            }
+          };
+
----------------
kaushikcfd wrote:
> jdoerfert wrote:
> > kaushikcfd wrote:
> > > jdoerfert wrote:
> > > > You need to verify there is only a single load/store and the value is an alloca without initializer and the type is as expected.
> > > > verify there is only a single load/store
> > > There might be multiple loads/store to `%omp.ub` arising from `EnsureUpperBound`. So, we only pick the store just before and the load just after the `static_for` invocation.
> > > 
> > > >  value is an alloca without initializer
> > > Is the "value" begin discussed here `V`?
> > If there are multiple, how do you ensure you pick up the right one? We probably need to find all and use the upper/lower bound of all stored values to be correct. We also need a test for multiple loads/stores.
> > 
> > I mean we assume `OMPLBVal` and `OMPUBVal` are alloca's, right? We need to make sure we only pattern match a situation we can "control"
> Agreed, the current implementation is broken. I've added a FIXME for it. Looks like we should do some dataflow analysis to figure out the correct range.
> 
> > I mean we assume OMPLBVal and OMPUBVal are alloca's, right? 
> Yep, made the assumption explicit in the latest revision.
No "FIXME" for correctness issues please. "FIXME" is fine for required improvements though.
So, the case we can handle correctly now we should. We should also detect if it is not one of them and give up, e.g., if there are more than a single load and store. I recommend just tracking all uses, determining if it is a load of the pointer or a store *to the pointer* (not of it), putting them in respective sets, and bailing on other uses. Then, inspect the loads/stores, for now just bail if the number is not one for each.

We should have a test with a FIXME as well to show what is missing.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:215
+
+    assert(R);
+
----------------
asserts need a message.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:235
+    bool Changed = false;
+    RuntimeFunctionInfo &RFI = RFIs[OMPRTL___kmpc_for_static_init_4];
+
----------------
In this patch or a follow up we need to do this for the other loop init runtime calls as well (there are 4 or 8 I think).


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:316
+              LoadFromUB->getMetadata(LLVMContext::MD_range), Bound))
+        LoadFromUB->setMetadata(LLVMContext::MD_range, Bound);
+
----------------
Style: I would prefer `if (is..) { changed = true; ... }` as it makes the code easier to read (for me).


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:318
+
+      NumOpenMPLoopsRangeAnnotated += Changed;
+
----------------
This will not count the right thing if the first loop is annotated but no other one. 


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

https://reviews.llvm.org/D75893