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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

In https://reviews.llvm.org/D48026#1143538, @philip.pfaffe wrote:

> For the sake of progress IMO it's okay to leave a TODO and address it in a followup change. @Meinersbur what do you think?


Agreed.



================
Comment at: lib/Support/ScopHelper.cpp:461
+
+    for (unsigned int Idx = 1; Idx < GepInst->getNumIndices(); Idx++) {
+      Value *Val = GepInst->getOperand(Idx);
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop | Do not evaluate something invariant in the loop exit condition ]]

[serious] Off-by-one-error: `getNumIndices()` returns `getNumOperands() - 1`, so you are skipping the last element here.

[style] Could use a foreach-loop: `for (const Use &Val : llvm::drop_begin(this->operands(), 1))`, but this could require D48598.


================
Comment at: lib/Support/ScopHelper.cpp:467-468
+      if (!SE.isLoopInvariant(PtrSCEV, OuterLoop)) {
+        IndexIsInvariant = false;
+        break;
+      }
----------------
[style] Instead of using a flag, this pattern of "find element that matches a condition" can be simplified by outlining the loop into a `hasVariantIndex` function that `return true;` if such an element is found and `return false;` after the loop. Alterntively, `std::any_of` with a lambda could be useful.

(I don't require this change for me accept this patch)


https://reviews.llvm.org/D48026