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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:249
+            }
+          };
+
----------------
kaushikcfd wrote:
> 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)
Sounds good :) I will monitor the reviews more closely now anyway.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:250
+        // Support a simple, but widely occurring case.
+        return false;
+
----------------
Style: I would move the comment before the `if` or add braces.

I'm curious, is this a widely occurring case? Do you know when it occurs in practice?


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

https://reviews.llvm.org/D75893