[polly] r221393 - BlockGenerator: Recompute values from SCEV before handing back the original values

[Tobias Grosser]

On 06.11.2014 18:35, Johannes Doerfert wrote:
> On 11/05, Tobias Grosser wrote:
>> Author: grosser
>> Date: Wed Nov  5 14:48:56 2014
>> New Revision: 221393
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=221393&view=rev
>> Log:
>> BlockGenerator: Recompute values from SCEV before handing back the original values
>>
>> This patch moves the SCEV based (re)generation of values before the checking for
>> scop-constant terms. It enables us to provide SCEV based replacements, which
>> are necessary to correctly generate OpenMP subfunctions when using the SCEV
>> based code generation.
>>
>> When recomputing a new value for a value used in the code of the original scop,
>> we previously directly returned the same original value for all scop-constant
>> expressions without even trying to regenerate these values using our SCEV
>> expression. This is correct when the newly generated code remains fully in the
>> same function, however in case we want to outline parts of the newly generated
>> scop into subfunctions, this approach means we do not have any opportunity to
>> update these values in the SCEV based code generation. (In the non-SCEV based
>> code generation, we can provide such updates through the GlobalMap). To ensure
>> we have this opportunity, we first try to regenerate scalar terms with our SCEV
>> builder and will only return scop-constant expressions if SCEV based code
>> generation was not possible.
> Is this the only solution we have? It seems this might have compile time
> effects as the early exits are moved.

Right. My guess was it will not be measurable. I just verified this. Our
buildbots do not report any changes in compile time for this commit:

http://llvm.org/perf/db_default/v4/nts/32833?num_comparison_runs=0&test_filter=&test_min_value_filter=&aggregation_fn=median&MW_confidence_lv=0.05&compare_to=32821&submit=Update

> Does this work if the value used
> in the subfunction is a induction variable of a loop in/outside of the
> SCoP? I looked into this problem a bit myself and I am not certain this
> is the best (or even a complete) way of handling the problem.

I extracted this change to make sure the single-thread and one-function
functionality remains unchanged. I believe this is the case.

The openmp patch I (re)submitted in http://reviews.llvm.org/D5517 has 
test coverage for the subfunction related issue.

>> This change should not affect the results of the existing code generation
>> passes. It only impacts the upcoming OpenMP based code generation.
>>
>> This commit also adds a test case. This test case passes before and after this
>> commit. It was added to ensure test coverage for the changed code.
> Whats the benefit here? As long as the code does not segfault it sounds
> like the test is not really helpful or is it?

During my testing I removed the following two lines entirely:

   // A scop-constant value defined by a global or a function parameter.
   if (isa<GlobalValue>(Old) || isa<Argument>(Old))
     return const_cast<Value *>(Old);

   // A scop-constant value defined by an instruction executed outside 
the scop.
   if (const Instruction *Inst = dyn_cast<Instruction>(Old))
     if (!Statement.getParent()->getRegion().contains(Inst->getParent()))
       return const_cast<Value *>(Old);

Without the test case, we did not see any test suite failures at all. To
not accidentally drop support for this, I added a test cae.

Cheers,
Tobias