[PATCH] D38403: [Polly][ScopBuilder] Introduce -polly-stmt-granularity=scalar-indep option.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/Analysis/ScopBuilder.cpp:711
+/// of two such instructions.
+static bool isOrderedInstruction(Instruction *Inst) {
+  return Inst->mayHaveSideEffects() || Inst->mayReadOrWriteMemory();
----------------
bollu wrote:
> Just to clarify: An Instruction `I` is ordered if there exists an instruction `J` such that `I; J != J; I`, correct?
Not exactly, but it's the right idea.

Nit: Equality (`!=`) is not the right relation. If `I;J` is a tuple,  then those are never equal. If it is a set, they are always equal. What you mean is some equivalence relation for semantics, which is difficult to define formally. I also don't intend to guarantee that if this function is true, that such an instruction `J` exists. Interactions with a third instruction `K` between `I` and `J` might also be possible, or depend on the code around. It is more that if it returns false and any permutation of non-ordered instructions have the same semantics in any context.

I'd stay with a verbal definition.


================
Comment at: lib/Analysis/ScopBuilder.cpp:739
+/// Join the PHI write epilogue if necessary.
+static void joinEpilogue(EquivalenceClasses<Instruction *> &UnionFind,
+                         ArrayRef<Instruction *> ModeledInsts, BasicBlock *BB) {
----------------
bollu wrote:
> Consider `joinPHIWritesToEpilogue`?
I call "epilogue" the last statement of a basic block where the PHI writes are added to. That is, PHI writes are always in the epilogue, whether this function is called or not.

More exact would be `joinStatementsContainingDefinitionsOfIncomingValuesInSuccessorBlockWithEpilogue`.

If PHI writes are added to the statement containing the definition instead of the writes, this function becomes useless. I hope to implement that sometime.


================
Comment at: lib/Analysis/ScopBuilder.cpp:799
+/// 'ordered instruction'.
+static void joinEpilogueLast(EquivalenceClasses<Instruction *> &UnionFind,
+                             ArrayRef<Instruction *> ModeledInsts) {
----------------
bollu wrote:
> Consider a name such as `joinAllAfterEpilogue`? I feel that this name carries the intent a little better.
Good idea.


================
Comment at: test/ScopInfo/granularity_scalar-indep.ll:4
+; Split a block into two independant statements that share no scalar.
+; This case has the instructions of the two statements interleanved, such that
+; splitting the BasicBlock in the middle would cause a scalar dependency.
----------------
grosser wrote:
> interleaved
thanks


================
Comment at: test/ScopInfo/granularity_scalar-indep_noepilogue.ll:3
+;
+; This case has no explicit epilogie for PHI writes because it would
+; have a scalar dependency to the previous statement.
----------------
bollu wrote:
> Nit: `epilogie` -> `epilogue`
thanks


https://reviews.llvm.org/D38403