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

[Kaushik Kulkarni via Phabricator via llvm-commits]

kaushikcfd marked an inline comment as not done.
kaushikcfd added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:249
+            }
+          };
+
----------------
jdoerfert wrote:
> 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.
The "FIXME" was meant to be handled before getting this merged in. (this is in process of being solved, will ping once I've put in the solution)


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

https://reviews.llvm.org/D75893