[PATCH] D118102: [LoopInterchange] Detect output dependency of a store instruction with itself

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/test/Transforms/LoopInterchange/lcssa-preheader.ll:5
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
-
-; void foo(int n, int m) {
----------------
bmahjour wrote:
> congzhe wrote:
> > bmahjour wrote:
> > > This is a valid test case to interchange, and looks like after this patch, it won't be interchanged anymore. I believe @congzhe is looking into seeing if we can treat scalar output dependencies as interchange-preventing so that we can still get scenarios like this one.
> > Thanks Bardia, likely I was not clear enough during our last discussion -- just to clarify the fact that this test case wont be interchanged after this patch is due to the insufficient capability of DependenceAnalysis/Delinearization which gives [* *] as the direction vector. Remember that we now detect output dependency and take the store instruction into consideration, which results in a direction vector of [* *]. IIUC this problem seems independent of the current patch -- this patch fixes the deficiency that output dependencies are not considered, if it exposes other problems in other aspects I'm thinking to fix them in other patches maybe?
> > 
> > Previously I thought to remove this test since it seems not very meaningful. But actually with a trivial change in the test (similar to the trival changes in the two other tests in this patch), DependenceAnalysis could work properly and this test can be interchanged. I'll update the patch accordingly. 
> > 
> > -----
> > 
> > Regarding your comment that `if we can treat scalar output dependencies as interchange-preventing so that we can still get scenarios like this one`: 
> > 
> > I would like to first clarify again that scalar dependency is not related to this test/scenario, what fails this test is that dependence analysis gives [* *] as the direction vector.
> > 
> > I've also followed your suggestion and checked that if we treat scalar output dependencies as interchange-preventing, we would fail the following 10 tests. I looked into a few of them and IMHO it seems that scalar output dependencies seem to be legal in terms of interchange. Please correct me if I'm wrong.
> > 
> > Failed Tests (10):
> >   LLVM :: Transforms/LoopInterchange/inner-only-reductions.ll
> >   LLVM :: Transforms/LoopInterchange/innermost-latch-uses-values-in-middle-header.ll
> >   LLVM :: Transforms/LoopInterchange/interchange-no-deps.ll
> >   LLVM :: Transforms/LoopInterchange/lcssa.ll
> >   LLVM :: Transforms/LoopInterchange/outer-header-jump-to-inner-latch.ll
> >   LLVM :: Transforms/LoopInterchange/phi-ordering.ll
> >   LLVM :: Transforms/LoopInterchange/pr43176-move-to-new-latch.ll
> >   LLVM :: Transforms/LoopInterchange/pr43797-lcssa-for-multiple-outer-loop-blocks.ll
> >   LLVM :: Transforms/LoopInterchange/reductions-across-inner-and-outer-loop.ll
> >   LLVM :: Transforms/LoopInterchange/vector-gep-operand.ll
> > 
> > ... this test case wont be interchanged after this patch is due to the insufficient capability of DependenceAnalysis/Delinearization which gives [* *] as the direction vector. Remember that we now detect output dependency and take the store instruction into consideration, which results in a direction vector of [* *].
> 
> Sure, DependenceAnalysis may have issues, but I'm wondering if those same "issues" are the reason this patch prevents interchange for the motivating test case. For instance, for `lcssa_08` the reason we see [* *] is because of delinearization issues which could be improved in future (you can test and verify this with `-da-disable-delinearization-checks`). I think keeping this test and adding `-da-disable-delinearization-checks` is better than removing it all together.
>  
> Also with `-da-disable-delinearization-checks` does the motivating test case still pass with this patch?
> 
> > I've also followed your suggestion and checked that if we treat scalar output dependencies as interchange-preventing, we would fail the following 10 tests. 
> 
> Thanks for trying this. 
> 
> > I looked into a few of them and IMHO it seems that scalar output dependencies seem to be legal in terms of interchange
> 
> Could you elaborate on why you think they should not be interchange preventing? This whole patch is premised on detecting output dependencies and treating them as interchange preventing.

>  I think keeping this test and adding -da-disable-delinearization-checks is better than removing it all together.

I fully agree, we can interchange this case after adding -da-disable-delinearization-checks. I'll update the patch shortly.

> I'm wondering if those same "issues" are the reason this patch prevents interchange for the motivating test case.

Just to clarify a bit, the current patch does not prevent interchange for any case, what this patch does is to fix the deficiency that we did not detect output dependency before. I'm hoping this is one step closer to solving the motivating bug.

> Could you elaborate on why you think they should not be interchange preventing? This whole patch is premised on detecting output dependencies and treating them as interchange preventing.

If we take a look at `no_mem_instrs()` in `Transforms/LoopInterchange/interchange-no-deps.ll`, the output dependency on `store i64 %indvars.iv, i64* %ptr, align 4` is a scalar output dependency and is supposed to be legal to interchange, if we treat scalar output dependencies as interchange-preventing, this test would fail. Similar situation occurs for `lcssa_05()` and `lcssa_06()` in `Transforms/LoopInterchange/lcssa.ll`.


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

https://reviews.llvm.org/D118102