[PATCH] D52116: Introduce llvm.loop.parallel_accesses and llvm.access.group metadata.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked 3 inline comments as done.
Meinersbur added a comment.

I am going to to prepare an RFC.



================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:631
     // Set "llvm.mem.parallel_loop_access" metaData to versioned loop.
+    // FIXME: "llvm.mem.parallel_loop_access" annotates memory access instructions, not loops.
     addStringMetadataToLoop(LVer.getVersionedLoop(),
----------------
hfinkel wrote:
> I don't understand what this FIXME is saying. What needs to be fixed?
The line below adds `llvm.mem.parallel_loop_access` to a LoopID, but is expected as annotations of instructions that access memory (with a the loop it is parallel to as parameter).

There is no code that looks for `llvm.mem.parallel_loop_access` in LoopID metadata. Hence, adding the property has no effect.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:808
+        // memory access instruction, but this would require updating all uses
+        // of one of the access groups in the function. Alternatively, we could
+        // create a new accesses group inheriting from the two others, but this
----------------
hfinkel wrote:
> Is the problem with "updating all uses of one of the access groups" that we don't have a way to efficiently enumerate them? Would we need to scan the functions for branches and collect all of the loop-id metadata that's relevant first? It would be nice not to lose this information.
Either search (and update) all LoopIDs that reference the access group or create a new 'meta-access-group' as outlined in the FIXME.

Of course it would be nice to not lose information, but as for any analyses there is a trade-off between accuracy and computational complexity. E.g. it would be nice if alias-analysis would be control-flow-sensitive. At there moment the only pass making use of `Loop::isAnnotatedParallel()` are LoopVectorize, LoopVersioningLICM, LoopDistribute and LoopLoadElimination. All of which only process innermost loops. That is, there would be no effect of keeping more information. There's also the FIXME still in the code such that the issue is not forgotten.

Do you still want me to implement one of the solutions?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52116/new/

https://reviews.llvm.org/D52116