From a1b97f007a1b50d93ec802d5aeb23de01a9fd119 Mon Sep 17 00:00:00 2001 From: Jan Vrany Date: Thu, 15 Aug 2024 21:18:37 +0100 Subject: [PATCH] RLSRA: fix edge case when (virtual) register is only defd but not used Consider following - somewhat pathological - TRIL fragment: N... N004 treetop N003 iadd N001 iload 'x' N002 iload 'y' N... such that the value of the `iadd` node (`N003`) is never used. The iadd node is compiled to (say) something like: addi {vr003}, {vr001}, {vr002} This instruction defines (assigns) a virtual register `{vr003}` but since its value is never used, the interval for `{vr003}` of length 1 and such interval was *not* created by interval splitting. Now consider even trickier example: N... N004 treetop N003 icall 'some_func' N... such that value of the icall node (`N003`) is never used just like in previous example (in other words: the code calls a function `some_func` but does not use its return value - an entirely valid, if not common, situation). Here, the call instruction does define (say) virtual register `{vr003}` by having a dependency of linkage-defined return register on `{vr003}`. But since value of `{vr003}` is never used, there's no need to generate register move from linkage-defined return register to `{vr003}`. This commit fixes RLSRA for cases like these. --- .../TRCompilationTestCase.class.st | 72 +++++++++++++++++++ src/Tinyrossa/TRRegisterLiveInterval.class.st | 29 ++++++-- ...everseLinearScanRegisterAllocator.class.st | 32 ++++++--- 3 files changed, 118 insertions(+), 15 deletions(-) diff --git a/src/Tinyrossa-Tests/TRCompilationTestCase.class.st b/src/Tinyrossa-Tests/TRCompilationTestCase.class.st index 971dd2b..7fb0185 100644 --- a/src/Tinyrossa-Tests/TRCompilationTestCase.class.st +++ b/src/Tinyrossa-Tests/TRCompilationTestCase.class.st @@ -252,6 +252,78 @@ TRCompilationTestCase >> test03_lconst [ " ] +{ #category : #tests } +TRCompilationTestCase >> test06_iadd_discarding_value [ + | x builder | + + + + x := testParameters at: #x. + + builder := compilation builder. + builder defineName: testSelector asString type: Int32. + builder defineParameter: 'x' type: Int32. + builder treetop: { + builder iadd: { + builder iload: 'x'. + builder iconst: 1 } }. + builder ireturn: + { builder iconst: -1 }. + + compilation optimize. + compilation compile. + + self assert: (shell call: x ) equals: -1. + + " + TRRV64GCompilationTests debug: #test06_iadd_discarding_value + TRPPC64CompilationTests debug: #test06_iadd_discarding_value + " +] + +{ #category : #tests } +TRCompilationTestCase >> test07_call_discarding_value [ + | x builder | + + + + self target name = 'powerpc64le-linux' ifTrue: [ + self skip: 'Skipped since Xload/Xstore evaluator does not support statics for POWER (see issue #52)'. + ]. + + x := testParameters at: #x. + + builder := compilation builder. + builder defineName: testSelector asString type: Int32. + builder defineParameter: 'x' type: Int32. + builder + if: (builder icmpeq: + { builder iload: 'x' . + builder iconst: 0 }) + then:[ :builder | + builder ireturn: + { builder iconst: -1 } ] + else:[ :builder | + builder icall: { + builder isub: + { builder iload: 'x' . + builder iconst: 1 } . + testSelector }. + builder ireturn: + { builder iload: 'x' } ]. + + compilation optimize. + compilation compile. + + + self assert: (shell call: x) equals: ((x == 0) ifTrue:[ -1 ] ifFalse:[ x ]). + + " + TRRV64GCompilationTests debug: #test07_call_discarding_value + TRPPC64CompilationTests debug: #test07_call_discarding_value + " +] + { #category : #'tests - examples' } TRCompilationTestCase >> test_example01_meaningOfLife [ TRCompilationExamples new diff --git a/src/Tinyrossa/TRRegisterLiveInterval.class.st b/src/Tinyrossa/TRRegisterLiveInterval.class.st index 1f5d82f..5be9aba 100644 --- a/src/Tinyrossa/TRRegisterLiveInterval.class.st +++ b/src/Tinyrossa/TRRegisterLiveInterval.class.st @@ -184,15 +184,36 @@ TRRegisterLiveInterval >> length [ ^ self stop - self start + 1 ] +{ #category : #queries } +TRRegisterLiveInterval >> needsToBeSpilled [ + "Return true, if this interval (register) has to be spilled after its definition" + ^ spillSlot notNil +] + { #category : #queries } TRRegisterLiveInterval >> needsToBeSpilledAt: position [ ^ spillSlot notNil and: [ position = self lastDef ] ] -{ #category : #queries } -TRRegisterLiveInterval >> needsToBeSpilled [ - "Return true, if this interval (register) has to be spilled after its definition" - ^ spillSlot notNil +{ #category : #accessing } +TRRegisterLiveInterval >> nextUseAfter: position [ + "Return the next (closest) use position for this interval greater than `position`. + If there's no use after given position and before next (closest) def position + return `nil`." + + | encodedDefPosition | + + encodedDefPosition := self encodeDefPosition: position. + uses do: [ :i | + encodedDefPosition < i ifTrue:[ + (self encodesUsePosition:i) ifTrue: [ + ^ self decodePosition: i. + ] ifFalse: [ + ^ nil + ]. + ]. + ]. + ^ nil. ] { #category : #'printing & storing' } diff --git a/src/Tinyrossa/TRReverseLinearScanRegisterAllocator.class.st b/src/Tinyrossa/TRReverseLinearScanRegisterAllocator.class.st index f9a64dd..80eb8d1 100644 --- a/src/Tinyrossa/TRReverseLinearScanRegisterAllocator.class.st +++ b/src/Tinyrossa/TRReverseLinearScanRegisterAllocator.class.st @@ -193,12 +193,19 @@ TRReverseLinearScanRegisterAllocator >> allocateRegistersAt: insnIndex [ deps post do: [:dep | dep isUnsatisfiedDependency ifTrue:[ - "Move value from real register to its virtual register." - self insertMoveFrom: dep rreg to: dep vreg. + "Move value from real register to its virtual register but only + if the value is needed (this is usually the case, but not always!)" + | interval | + + interval := live detect: [:each | each register == dep vreg ] ifNone: [ nil ]. + (interval notNil and:[(interval nextUseAfter: insnIndex) notNil]) ifTrue: [ + self insertMoveFrom: dep rreg to: dep vreg. + ]. live copy do: [:i | (i register allocation == dep rreg) ifTrue: [ - "Live-across register is trashed, we have to spill and reload. + "If any live register is allocated to dependency's real register + (which is trashed at this point) we have to spill and reload. We do it by forcefully splitting the interval." self splitRegister: i at: insnIndex. ]. @@ -231,14 +238,17 @@ TRReverseLinearScanRegisterAllocator >> allocateRegistersAt: insnIndex [ self allocateRegister: interval. interval length == 1 ifTrue: [ "We have just allocated an register interval of length 1 - - such interval may be result of a split between its - definition and first use. In this case, this interval is - defined at this interval get immediatelly spilled so - we can expire it right now to free the register for - possibly other intervals that go live here." - self assert: (interval needsToBeSpilledAt: insnIndex). - - self insertSpill: interval. + such interval may be result certain TRIL (like a call to + non-void function whose return value is not used) or it may + be result of a split between its definition and first use. + If the latter, this interval is defined at this interval + must be immediatelly spilled. + + In both cases, so we can expire it right now to free the + register for possibly other intervals that go live here." + (interval needsToBeSpilledAt: insnIndex) ifTrue: [ + self insertSpill: interval. + ]. self expireRegister: interval. ] ].