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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

All tests passed for me. I also tried your test case with `-polly-use-runtime-alias-checks=false -polly-ignore-aliasing` and it still works with codegen (I had doubts that if your additional code is not executing, the chain of address loads would not end up in `RequiredILs`).

Before the patch is good to be landed, please address the [serious] remark.



================
Comment at: lib/Analysis/ScopDetection.cpp:1146
       bool CanBuildRunTimeCheck = true;
+
       // The run-time alias check places code that involves the base pointer at
----------------
[nit] Unrelated change


================
Comment at: lib/Support/ScopHelper.cpp:445-446
 
+bool polly::hasVariantIndex(GetElementPtrInst *Gep, Loop *L, Region &R,
+                            ScalarEvolution &SE) {
+  for (unsigned int Idx = 1, IdxEnd = Gep->getNumOperands(); Idx < IdxEnd;
----------------
[remark] `hasVariantIndex` is only used in this file, so a declaration `static bool hasVariantIndex(...` (or a lambda) without anything added to the header file would have been enough.



================
Comment at: lib/Support/ScopHelper.cpp:452-454
+    if (!SE.isLoopInvariant(PtrSCEV, OuterLoop)) {
+      return true;
+    }
----------------
[nit] We usually do not enclose a single statement following an `if` in braces.


================
Comment at: test/ScopDetect/collective_invariant_loads.ll:1
+; RUN: opt %s -polly-scops -polly-invariant-load-hoisting -analyze 2>&1| FileCheck %s
+
----------------
[serious] `%loadPolly` missing

[style] [[ https://www.llvm.org/docs/TestingGuide.html#fragile-tests | Use `< %s` ]]

[suggestion] `2>&1` is unnecessary


================
Comment at: test/ScopDetect/collective_invariant_loads.ll:22
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
[style] The `target triple`. `ModuleID` and `source_filename` lines can be removed; The `target triple` in particular can cause the test to fail if `LLVM_TARGETS_TO_BUILD` does not include X86.


https://reviews.llvm.org/D48026