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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.

I tried to make a test case for what I tried to explain during the phone call: F6484800: collective_invariant_loads2.ll <https://reviews.llvm.org/F6484800>

The different address levels are different alias sets due to the `-scoped-noalias` metadata. These have to be MayAlias sets otherwise there is no reason to check for aliasing (this are what the dummy loads are for). Next, the last load (`%tmp9`) has to be processed first, before the load its address depends on (`%tmp5`) is known to be invariant. I put the loads in reverse order in the source to be iterated over in the order reverse of how it is executed.

Unfortunately this does not work, the block iteration comes from `RegionInfo`, which iterates in depth-first order. This should guarantee that dominators (where the invariant address is loaded) are visited before the block that loads from that address. This is, unless we also support invariant PHINodes, there is probably no counterexample. Still, depending in the check order is not robust, but no pressing issue.

I found a correctness issue which I marked with [serious]. Because of this, I mark the patch as "request changes" to overwrite the "Accepted" status.



================
Comment at: lib/Analysis/ScopDetection.cpp:1154
+
+      while (1) {
+        const unsigned int VariantSize = VariantLS.size(),
----------------
Could you add comment(s) for this code as well?


================
Comment at: lib/Support/ScopHelper.cpp:449-450
   auto *Ptr = LInst->getPointerOperand();
+
+
+  // Before deciding for a load to be invariant, we check if the previous
----------------
`polly-check-format` will complain about two consecutive empty lines.


================
Comment at: lib/Support/ScopHelper.cpp:451-453
+  // Before deciding for a load to be invariant, we check if the previous
+  // load (on which the current load instruction depends) is marked
+  // as invariant. 
----------------
Could you reformulate this? It sounds like the previous load being invariant is a necessary condition. Also, could you add a TODO about avoiding the pattern matching.

Suggestion:
> An LoadInst is hoistable if the address it is loading from is also invariant; in this case: another invariant load (whether that address is also not written to has to be checked separately). 
> TODO: The only checks for a LoadInst->GetElementPtrInst->LoadInst pattern generated by the Chapel frontend, but generally this applies for any chain of instruction that does not also depend on any induction variable.

[serious] While writing this, I noticed that the pattern checked is not sufficient. The index argument could be an induction variable, meaning that the address loaded from changes in every iteration, i.e. the load is not invariant.




https://reviews.llvm.org/D48026