[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:221
+      Value *OMPLBVal = staticForCall->getArgOperand(4);
+      Value *OMPUBVal = staticForCall->getArgOperand(5);
+
----------------
jdoerfert wrote:
> We need names for these "magic constants" maybe some globals like this:
> ```
> static constexpr unsigned KMPC_FOR_STATIC_INIT_LB = 4;
> static constexpr unsigned KMPC_FOR_STATIC_INIT_UB = 5;
> ```
move them into the class or global scope.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:249
+            }
+          };
+
----------------
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"


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:47
+STATISTIC(NumOpenMPLoopsRangeAnnotated,
+          "Number of OpenMP for directives annotated with !range");
 
----------------
-for
+worksharing



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:240
+
+        for (; I != E; ++I) {
+          Value *V = I->getUser();
----------------
sstefan1 wrote:
> `for (const Use &U : BoundVal->uses())` seems nicer.
Right. And if you only want the users: `users()`.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:267
+        return false;
+      } else {
+        LLVMContext &Context = StaticForCall->getParent()->getContext();
----------------
no `else` after `return`


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:272
+        ConstantInt *HighMinusOne =
+            dyn_cast<ConstantInt>(StoreToUB->getValueOperand());
+        ConstantInt *High = ConstantInt::get(
----------------
Either move these `dyn_cast` prior to the conditional and branch on `Low` and `HighMinusOne` (which are nullptr if they are not constant ints), or use `cast` here because you had a `isa` before. I would probably do the former.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:280
+        };
+        LoadFromLB->setMetadata(LLVMContext::MD_range,
+                                MDNode::get(Context, LowAndHigh));
----------------
We should check that this is better than the existing metadata.


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

https://reviews.llvm.org/D75893