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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a subscriber: sstefan1.
jdoerfert added a comment.

This looks pretty good already. I inlined some comments. We also need negative tests, e.g., with non-constant loop bounds.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:46
           "Number of OpenMP runtime function uses identified");
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
----------------
We should add a statistics tracker for annotated loops.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:105
     }
   };
 
----------------
The documentation here needs improvement. If the callback returns `true` the use will be deleted. Could you augment the documentation and make sure you don't cause uses to be removed from the cache.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:218
+      if (!staticForCall)
+        return false;
+
----------------
We still use capital first letters for characters, thus `SetRangeCB` and `StaticForCall`, ...


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:221
+      Value *OMPLBVal = staticForCall->getArgOperand(4);
+      Value *OMPUBVal = staticForCall->getArgOperand(5);
+
----------------
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;
```


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:224
+      StoreInst *storeToLB = NULL, *storeToUB = NULL;
+      LoadInst *loadFromLB = NULL, *loadFromUB = NULL;
+
----------------
Nit: I somehow would prefer `nullptr` instead of `NULL`.


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


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:278
+            << " as the iteration zone of '" << *staticForCall
+            << "' isn't compile time constant.\n");
+
----------------
Style: Early exists are easier to read, so:
```
if (cond) {
  DEBUG(...);
  return;
}
```


================
Comment at: llvm/test/Transforms/OpenMP/set_bound_ranges.ll:100
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 11.0.0 (git at github.com:llvm/llvm-project 7264cf4e457e759a84bcac45882cad50628dbc15)"}
+; CHECK: ![[RANGE0]] = !{i32 0, i32 196}
----------------
I think you can remove the attributes and metadata above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75893