[PATCH] D48026: [ScopHelper] Provide support for recognising collective invariant loads
Michael Kruse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 26 10:24:09 PDT 2018
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
More information about the llvm-commits
mailing list