[PATCH] Add a case to LiveIntervalAnalysis::HandleMoveUp

[Andrew Trick]

On Mar 12, 2013, at 10:54 AM, Vincent Lejeune <vljn at ovi.com> wrote:

> I ran with both options and get no regressions.
> 
> The execution_time tests that fail with and without my patches are the following ones by the way : 
> 
> --- Tested: 986 tests --
> FAIL: MultiSource/Applications/Burg/burg.execution_time (494 of 986)
> FAIL: MultiSource/Applications/ClamAV/clamscan.execution_time (495 of 986)
> FAIL: MultiSource/Applications/lemon/lemon.execution_time (496 of 986)
> FAIL: MultiSource/Benchmarks/MiBench/automotive-bitcount/automotive-bitcount.execution_time (497 of 986)
> FAIL: MultiSource/Benchmarks/MiBench/telecomm-FFT/telecomm-fft.execution_time (498 of 986)
> FAIL: MultiSource/Benchmarks/Olden/voronoi/voronoi.execution_time (499 of 986)
> FAIL: MultiSource/Benchmarks/Ptrdist/anagram/anagram.execution_time (500 of 986)
> FAIL: SingleSource/Benchmarks/BenchmarkGame/puzzle.execution_time (501 of 986)

Thanks Vincent. I don't see these failures. Your test configuration must be different. If they fail with ToT llvm and test-suite (make sure to update your tests periodically), then it's worth filing a bug.

I'm usually testing (not benchmarking) with:

'make' 'SMALL_PROBLEM_SIZE=1' 'report' 'report.csv' '-j8' 'TEST=simple' 'TARGET_FLAGS=-arch x86_64 -Wno-error=implicit-function-declaration' 'OPTFLAGS=-O3' 'ARCH=x86_64' 'CC_UNDER_TEST_TARGET_IS_X86_64=1' 'CLANGPATH=/b/test/fix-x86/bin/clang' 'CLANGXXPATH=/b/test/fix-x86/bin/clang++' 'LLVMCC_OPTION=clang' 'CC_UNDER_TEST_IS_CLANG=1' 'DISABLE_CBE=1' 'DISABLE_JIT=1' 'ENABLE_PARALLEL_REPORT=1' 'ENABLE_HASHED_PROGRAM_OUTPUT=1' 'USE_REFERENCE_OUTPUT=1'

If you're not using reference output, it could actually be a bug with your reference compiler.

-Andy

> ----- Mail original -----
>> De : Andrew Trick <atrick at apple.com>
>> À : Vincent Lejeune <vljn at ovi.com>
>> Cc : Jakob Stoklund Olesen <stoklund at 2pi.dk>; Commit Messages and Patches for LLVM <llvm-commits at cs.uiuc.edu>
>> Envoyé le : Mardi 12 mars 2013 17h18
>> Objet : Re: [PATCH] Add a case to LiveIntervalAnalysis::HandleMoveUp
>> 
>> 
>> On Mar 12, 2013, at 7:26 AM, Vincent Lejeune <vljn at ovi.com> wrote:
>> 
>>> Hi Jacob and Andrw,
>>> 
>>> I reworked my patch so that now it does not change the value of a live out 
>> value. It also keep live in value.
>>> The 2 others patches enable mov of instructions that access sub-register 
>> (needed to test the feature).
>>> 
>>> I added some tests using -verify-misched in a previous patch : 
>>> 
>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130311/167949.html
>>> They come from piglit (the test suite we use for opengl) and failed often 
>> when I was working on handlemove of subregs,
>>> I think they would be good candidate for unit testing the feature.
>>> 
>>> There are also 4 unit tests in the third patches, these are simple modules 
>> which generated not optimal code
>>> without recomputing the schedule graph ; the tests check now that they do.
>>> 
>>> Thank for your help in getting these patches in a proper form (although I 
>> think they need reviews) and for your patience.
>>> Vincent
>> 
>> Thanks Vincent,
>> 
>> The scheduler patches look great. We can find ways to reduce compile time of 
>> recomputing dependencies eventually, feel free to file a bug if it's a 
>> problem.
>> 
>> Jakob should review the handleMoveUp/Down logic. To verify I suggest running the 
>> llvm test-suite with
>> 
>> -enable-misched -verify-misched -misched=shuffle
>> 
>> which I think you've done already. But additionally, to cover both 
>> handleMoveUp/Down as much as possible, you can add the options
>> 
>> -misched-topdown
>> 
>> and
>> 
>> -misched-bottomup
>> 
>> -Andy
>> 
>>> ----- Mail original -----
>>>> De : Jakob Stoklund Olesen <stoklund at 2pi.dk>
>>>> À : Vincent Lejeune <vljn at ovi.com>
>>>> Cc : Andrew Trick <atrick at apple.com>; Commit Messages and Patches 
>> for LLVM <llvm-commits at cs.uiuc.edu>
>>>> Envoyé le : Mercredi 6 mars 2013 23h11
>>>> Objet : Re: [PATCH] Add a case to LiveIntervalAnalysis::HandleMoveUp
>>>> 
>>>> 
>>>> On Mar 6, 2013, at 2:01 PM, Vincent Lejeune <vljn at ovi.com> wrote:
>>>> 
>>>>> Hi Jakob,
>>>>> 
>>>>>> 
>>>>>> Did you read my previous mail about the problem with live-out 
>> values? 
>>>> You 
>>>>>> didn't address the problem.
>>>>> 
>>>>> I actually don't understand what should be done for live out 
>> values.
>>>>> For instance if I have the following LiveInterval :
>>>>> 
>>>>> 
>>>> 
>> [48r,64r:2)[64r,80r:3)[80r,96B:4)[96B,336r:0)[448B,640r:0)[640r,672r:1)[672r,688r:5)[688r,704r:6)[704r,816B:7)
>>>>> With block boundaries :
>>>>> [0B;80B][96B;240B][256B;432B][448B;800B]
>>>>> My reasoning is the following :
>>>>> LiveRange that represents live-out value in the first bloc is 
>> [80r;96B] ;
>>>>> If I move 80r to 8B for instance, I'm expecting the the first 
>> three 
>>>> LiveRange to be :
>>>>> [8r,48r:4)[48r,64r:2)[64r,96B:3)
>>>>> ie val 3 is becoming the new live-out value, that ends where value 
>> 4 ended 
>>>> before.
>>>> 
>>>> The point is that the LiveInterval representation of liveness is just a 
>> 
>>>> compression method. A single LiveRange object can span multiple basic 
>> blocks, 
>>>> but for values that are live in more than one basic block, there is no 
>> guarantee 
>>>> that it only appears in a single LiveRange object. That depends on the 
>> block 
>>>> layout which you can't make assumptions about.
>>>> 
>>>> To avoid scanning the whole LiveInterval looking for instances of the 
>> escaped 
>>>> value, I would suggest that you change your code so the value number 
>> escaping 
>>>> the basic block doesn't change.
>>>> 
>>>> That would mean that the escaping value number is now defined by a 
>> different 
>>>> instruction - the one that is now the last def in the block.
>>>> 
>>>> /jakob
>>> 
>> <0001-Add-a-case-to-LiveIntervalAnalysis-HandleMove.patch><0003-R600-Recompute-schedule-graph-non-order-dependencies.patch><0002-R600-Use-bottom-up-scheduling-algorithm.patch>
>>