[PATCH] Add a case to LiveIntervalAnalysis::HandleMoveUp

Tue Mar 12 15:55:53 PDT 2013

I found this thread that may be related to my issue : 

http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-January/058508.html


----- 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 21h25
> Objet : Re: [PATCH] Add a case to LiveIntervalAnalysis::HandleMoveUp
> 
> 
> 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>
>>> 
>