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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
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(),
----------------
Meinersbur wrote:
> 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.
OIC, okay. Thanks.


================
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
----------------
Meinersbur wrote:
> 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?
> 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. 

Clearly I know this ;)

>  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.

Two things: First, this might be true today, but very soon won't be true (vectorization will soon handle outer loops, and we are developing other loop-nest transformations) and if we do the right thing now, we won't later need to go back and fix this later. Second, while it's true that full loop unrolling generally happens before inlining, we could have a loop that, at the point of inlining is an inner loop, but is later fully unrolled such that before vectorization (etc.) the loop here might become, once again, the inner loop.

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

Yes.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D52116