[PATCH] D63305: Propagate Trip count Assumptions to runtime check

[Sameer AbuAsal via Phabricator via llvm-commits]

sabuasal added inline comments.


================
Comment at: test/ScopDetect/model-dowhile-update-rtc.ll:33
+; NOFIXRTC: [p_0] -> {  : -2147483648 <= p_0 <= 2147483647 }
+; NOFIXRTC: if (p_0 >= 5 && 0 == (p_0 <= -1 || p_0 == 2147483647) && (&MemRef0[p_0 + 1] <= &MemRef1[5] || &MemRef1[p_0 + 1] <= &MemRef0[5]))
+
----------------
Meinersbur wrote:
> sabuasal wrote:
> > Meinersbur wrote:
> > > `p_0 >= 5 && !(p_0 <= -1 || p_0 == 2147483647)` is equivalent to `p_0 >= 5 && p_0 > -1 && p_0 != 2147483647` which could be simplified to `p_0 >= 5 && p_0 != 2147483647`, ie the FIXRTC case. I guess this is a consequence of having split `InvalidContext` and `AssumedContext` to avoid a `complement()`.
> > > 
> > > I'd prefer some more intelligent treatment of the `InvalidContext` and `AssumedContext` handling over special handling of `UPPERBOUND` assumptions.
> > > 
> > > The `buildMinMaxAccess` change is not reflected in this test case.
> > > 
> > > I'd prefer some more intelligent treatment of the InvalidContext and AssumedContext handling over special handling of UPPERBOUND assumptions.
> > > 
> > 
> > Can you please clarify, are you asking for a different way to propagate the assumptions that don't involve simply intersecting them?
> > 
> > 
> One could determine whether a `complement()` would actually be expensive. E.g. for `p_0 >= 5` we have have choices:
> 1. Add `p_0 >= 5` to `AssumedContext`
> 2. Add `p_0 < 5` to `InvalidContext`
> 
> For this case, they are equally complex, so we could prefer one over the other.
> 
> Another idea is to apply `InvalidContext = isl_set_gist(InvalidContext, AssumedContext)` that theoretically would remove unnecessary conditions from `InvalidContext`.
> 
> Can you confirm that the separation of InvalidContext/AssumedContext is the issue? I am currently just assuming that.
> The buildMinMaxAccess change is not reflected in this test case.
> 
Thanks for the catch. First,, this is no longer needed since the buildMinMaxAccess already uses the updated domains for the statements (the ones that got intersected and gisted with RestrictDomainParams). Also, I updated the test case to show the effect on the AliasGroup.


> For this case, they are equally complex, so we could prefer one over the other.
> 
Thanks for the explanation, I agree and I did it using by modifying the first suggestion (in the parent patch). In this patch I just want the Invalid Context to look a bit more simplified.




> 
> Another idea is to apply InvalidContext = isl_set_gist(InvalidContext, AssumedContext) that theoretically
> would remove unnecessary conditions from InvalidContext.
> 
I agree. I am just not sure it falls within the Scop of this patch. Maybe in a later patch since I think such a change will update lots of test cases as well.





> Can you confirm that the separation of InvalidContext/AssumedContext is the issue? I am currently just assuming that.
> 
I confirm. We build  these two context separately then we generate an AST in:


> IslAst::buildRunCondition(Scop &S, __isl_keep isl_ast_build *Build) {
>   isl_ast_expr *RunCondition;
> 
That is why the AST we get is not the most simplified.  My goal from the test case it to show that propagating the info to InvalidContext will make it a bit cleaner.




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

https://reviews.llvm.org/D63305