[PATCH] D48026: [ScopHelper] Provide support for recognising collective invariant loads

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

[serious] Could you add a test-case?

If I understand this correctly, this matches a specific pattern that mark a load as invariant if its address is also invariant.

[serious] As implemented, it is not stable since it depends on the order of in which LoadInsts are added to `RequiredILS`/`isHoistableLoad` being called. I.e. if `isHoistableLoad` is called on an address which has not yet been added to `RequiredILS`, it will be rejected.

[serious] It is also very specific to the address being computed using a single GEP instruction. Consider using ScalarEvolution to see that the address does not depend on an iteration counter or instruction inside the loop.



================
Comment at: include/polly/Support/ScopHelper.h:394
 /// @param DT    The dominator tree of the function.
 ///
 /// @return True if @p LInst can be hoisted in @p R.
----------------
[serious] The doxygen comment should also cover the added parameter.


================
Comment at: lib/Support/ScopHelper.cpp:450
+
+  auto *ptr_operand = dyn_cast<Instruction>(Ptr);
+  if (ptr_operand && isa<GetElementPtrInst>(ptr_operand)) {
----------------
[style] Please name variable according to the [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | LLVM coding standard ]].

Consider adding a comment about what this code is doing.


================
Comment at: lib/Support/ScopHelper.cpp:451
+  auto *ptr_operand = dyn_cast<Instruction>(Ptr);
+  if (ptr_operand && isa<GetElementPtrInst>(ptr_operand)) {
+    auto *prev_inst = dyn_cast<Instruction>(ptr_operand->getOperand(0));
----------------
[style] `if (auto *prev_inst = dyn_cast<GetElementPtrInst>(Ptr))`


================
Comment at: lib/Support/ScopHelper.cpp:455
+      auto *lt = dyn_cast<LoadInst>(prev_inst);
+      for (int i = 0; i < RequiredILS.size(); i++)
+        if (RequiredILS[i] == lt)
----------------
[style] Use a C++ foreach or a standard library function.


Repository:
  rPLO Polly

https://reviews.llvm.org/D48026