From 9fa36d90833b2ed14c9c69b7f44317619392b7bf Mon Sep 17 00:00:00 2001 From: Jan Vrany Date: Thu, 15 Aug 2024 18:30:35 +0100 Subject: [PATCH 1/3] Anchor calls to a treetop when building TRIL using `TRILBuilder` This commit changes the behavior of `TRILBuilder` w.r.t building calls. Since calls (`icall`, `acall` and so on) are not treetops, they were not added to the block. Therefore if the result of the call (return value) was not used, the call was elided from built IL. This is wrong, call may have side-effects so they must not be elided. This commit changes the behavior of `TRILBuilder` so that each call is automatically anchored using `treetop`. For example, following: builder istore: { builder icall: { 'get_some_int' } . 'temp' } used to produce: N002 istore 'temp' N001 icall 'get_some_int' With this commit, it produces: N002 treetop N001 icall 'get_some_int' N003 istore 'temp' N001 ==> icall 'get_some_int' If ever we really want to elide unnecessary calls (whateber that means) it should be implemented as an optimization pass. --- src/Tinyrossa-Tests/TRILSimplifierTests.class.st | 8 ++++---- src/Tinyrossa/TRCompilationExamples.class.st | 6 +----- src/Tinyrossa/TRILBuilder.class.st | 13 +++++++++++++ 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/Tinyrossa-Tests/TRILSimplifierTests.class.st b/src/Tinyrossa-Tests/TRILSimplifierTests.class.st index 2b5b0f3..7964117 100644 --- a/src/Tinyrossa-Tests/TRILSimplifierTests.class.st +++ b/src/Tinyrossa-Tests/TRILSimplifierTests.class.st @@ -117,11 +117,11 @@ TRILSimplifierTests >> test_simplify_store_02 [ b ireturn: { b iconst: 2 }. - self assert: compilation cfg treetops size == 4. - self assert: compilation cfg treetops second opcode = istore. + self assert: compilation cfg treetops size == 5. + self assert: compilation cfg treetops third opcode = istore. compilation optimize. - self assert: compilation cfg treetops size == 4. - self assert: compilation cfg treetops second opcode = treetop. + self assert: compilation cfg treetops size == 5. + self assert: compilation cfg treetops third opcode = treetop. ] diff --git a/src/Tinyrossa/TRCompilationExamples.class.st b/src/Tinyrossa/TRCompilationExamples.class.st index 5cdffef..8b643eb 100644 --- a/src/Tinyrossa/TRCompilationExamples.class.st +++ b/src/Tinyrossa/TRCompilationExamples.class.st @@ -687,11 +687,7 @@ TRCompilationExamples >> example17_call_external_function_in_aot_mode [ (builder defineFunction: 'exit' type: Void). - "Note, what we need to anchor call in a treetop otherwise - it would not be referenced from tree. Future versions of IL builder - may do this automatically." - - builder treetop: { builder call: { builder iload: 'status' . 'exit' } }. + builder call: { builder iload: 'status' . 'exit' }. builder ireturn: { builder iconst: 0 }. compilation config diff --git a/src/Tinyrossa/TRILBuilder.class.st b/src/Tinyrossa/TRILBuilder.class.st index 6c77343..5df4159 100644 --- a/src/Tinyrossa/TRILBuilder.class.st +++ b/src/Tinyrossa/TRILBuilder.class.st @@ -43,6 +43,19 @@ TRILBuilder >> build: opcode arguments: arguments [ node := super build: opcode arguments: arguments. node location: location. + + "If the node is call, we anchor it here under a treetop. + This is neccessary because otherwise, the node would be + elided if nobody uses the return value and we cannot elide + calls because they may have sideeffects. + + If we really want to elide calls to sideffect-free functions + (whateber that means) whose value are not used, it should be + done as an optimization pass." + opcode isCall ifTrue: [ + current add: (TRILNode opcode: treetop children: { node }) + ]. + ^ node ] From 6402474620bce2c1038d5dc1ff5dd1d5426218f3 Mon Sep 17 00:00:00 2001 From: Jan Vrany Date: Thu, 15 Aug 2024 21:34:24 +0100 Subject: [PATCH 2/3] RLSRA: strengthen invariants of (collected) register live intervals This strengthen invariant checking before proceeding to actual allocation, namely: * checks that each virtual register is at least once defined (assigned) and * checks that each virtual register is used only after it is defined or not used at all. --- src/Tinyrossa/TRRegisterLiveInterval.class.st | 12 ++++++++++++ .../TRReverseLinearScanRegisterAllocator.class.st | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/Tinyrossa/TRRegisterLiveInterval.class.st b/src/Tinyrossa/TRRegisterLiveInterval.class.st index 269eabe..1f5d82f 100644 --- a/src/Tinyrossa/TRRegisterLiveInterval.class.st +++ b/src/Tinyrossa/TRRegisterLiveInterval.class.st @@ -99,6 +99,18 @@ TRRegisterLiveInterval >> firstDef [ ^ nil ] +{ #category : #accessing } +TRRegisterLiveInterval >> firstUse [ + "Return the first use position for this interval." + + uses do: [:encodedPosition | + (self encodesUsePosition: encodedPosition) ifTrue: [ + ^ self decodePosition: encodedPosition + ]. + ]. + ^ nil +] + { #category : #initialization } TRRegisterLiveInterval >> initializeWithRegister: aTRVirtualRegister [ self assert: aTRVirtualRegister isTRVirtualRegister. diff --git a/src/Tinyrossa/TRReverseLinearScanRegisterAllocator.class.st b/src/Tinyrossa/TRReverseLinearScanRegisterAllocator.class.st index 73d8d13..f9a64dd 100644 --- a/src/Tinyrossa/TRReverseLinearScanRegisterAllocator.class.st +++ b/src/Tinyrossa/TRReverseLinearScanRegisterAllocator.class.st @@ -142,7 +142,9 @@ TRReverseLinearScanRegisterAllocator >> allocateRegisters [ ]. ]. intervals do: [:interval | - self assert: interval start < interval stop. + self assert: interval start <= interval stop. + self assert: interval firstDef notNil description: 'virtual register not defined (assigned)'. + self assert:(interval firstUse isNil or:[interval firstDef < interval firstUse]) description: 'virtual register used before defined (assigned)'. ]. "Create todo (work) list. The list is sorted by interval's end position (#stop). From a1b97f007a1b50d93ec802d5aeb23de01a9fd119 Mon Sep 17 00:00:00 2001 From: Jan Vrany Date: Thu, 15 Aug 2024 21:18:37 +0100 Subject: [PATCH 3/3] 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. ] ].