Merge pull request #53 from janvrany/pr/fix-rlsra-for-intervals-of-le…

…ngth-1

Fix RLSRA for intervals of length 1

src/Tinyrossa-Tests/TRCompilationTestCase.class.st

@@ -252,6 +252,78 @@ TRCompilationTestCase >> test03_lconst [
  "
 ]
 
+{ #category : #tests }
+TRCompilationTestCase >> test06_iadd_discarding_value [
+ | x builder |
+
+ <parameter: #x values: #(0 10)>
+
+ 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 |
+
+ <parameter: #x values: #(0 10)>
+
+ 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

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.
 ]

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

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
 ]
 

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.
@@ -172,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' }

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).
@@ -191,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.
  ].
@@ -229,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.
  ]
  ].