Skip to content
This repository
Browse code

SI-5313 Eliminate more stores by replacing clobbers with null stores

When an unused store clobbers a previous store, replace it with storing
a null. Don't mark clobbers as "used" so that the original clobber and
all following clobbers can still be eliminated.
  • Loading branch information...
commit 9b4fa8382fe0ed6fef3ff91e2c153a1840c954b9 1 parent eab2884
James Iry JamesIry authored
128 src/compiler/scala/tools/nsc/backend/opt/DeadCodeElimination.scala
@@ -68,6 +68,9 @@ abstract class DeadCodeElimination extends SubComponent {
68 68
69 69 /** Map from a local and a basic block to the instructions that store to that local in that basic block */
70 70 val localStores = mutable.Map[(Local, BasicBlock), mutable.BitSet]()
  71 +
  72 + /** Stores that clobber previous stores to array or ref locals. See SI-5313 */
  73 + val clobbers = mutable.Set[(BasicBlock, Int)]()
71 74
72 75 /** the current method. */
73 76 var method: IMethod = _
@@ -80,6 +83,7 @@ abstract class DeadCodeElimination extends SubComponent {
80 83 debuglog("dead code elimination on " + m);
81 84 dropOf.clear()
82 85 localStores.clear()
  86 + clobbers.clear()
83 87 m.code.blocks.clear()
84 88 accessedLocals = m.params.reverse
85 89 m.code.blocks ++= linearizer.linearize(m)
@@ -201,58 +205,15 @@ abstract class DeadCodeElimination extends SubComponent {
201 205 worklist += ((bb1, idx1))
202 206 }
203 207
204   - // Storing to local variables of reference or array type may be indirectly
205   - // observable because may remove a reference to an object which may allow the object to be
206   - // gc'd. See SI-5313. In this code I call the LOCAL_STORE(s) that immediately follow a
207   - // LOCAL_STORE and that store to the same local "clobbers." If a LOCAL_STORE is marked
208   - // useful then its clobbers must also go into the worklist to be marked useful.
209 208 case STORE_LOCAL(l1) if l1.kind.isRefOrArrayType =>
210 209 addDefs()
211   - // previously visited blocks tracked to prevent searching forever in a cycle
212   - val inspected = mutable.Set[BasicBlock]()
213   - // our worklist of blocks that still need to be checked
214   - val blocksToBeInspected = mutable.Set[BasicBlock]()
215   -
216   - // Tries to find the next clobber of l1 starting at idx1 in bb1.
217   - // if it finds any it adds them clobber to the worklist.
218   - // If not it adds both bb's exception blocks and direct
219   - // successor blocks to the uninspectedBlocks
220   - def findClobber(idx1: Int, bb1: BasicBlock) {
221   - val key = ((l1, bb1))
222   - val foundClobber = (localStores contains key) && {
223   - def minIdx(s : mutable.BitSet) = if(s.isEmpty) -1 else s.min
224   -
225   - // find the smallest index greater than or equal to idx1
226   - val clobberIdx = minIdx(localStores(key) dropWhile (_ < idx1))
227   - if (clobberIdx == -1)
228   - false
229   - else {
230   - debuglog(s"\t${bb1(clobberIdx)} is a clobber of $instr")
231   - if (!useful(bb1)(clobberIdx)) worklist += ((bb1, clobberIdx))
232   - true
233   - }
234   - }
235   -
236   - // if we found a clobber in this block then we don't need to add the successors
237   - // because they'll be picked up when the worklist comes around to work on that clobber
238   - if (!foundClobber) {
239   - blocksToBeInspected ++= (bb1.exceptionSuccessors filterNot inspected)
240   - blocksToBeInspected ++= (bb1.directSuccessors filterNot inspected)
241   - }
242   - }
243   -
244   - // first search starting at the current index
245   - // note we don't put bb in the inspected list yet because a loop may later force
246   - // us back around to search from the beginning of bb
247   - findClobber(idx + 1, bb)
248   - // then loop until we've exhausted the set of uninspected blocks
249   - while(!blocksToBeInspected.isEmpty) {
250   - val bb1 = blocksToBeInspected.head
251   - blocksToBeInspected -= bb1
252   - inspected += bb1
253   - findClobber(0, bb1)
254   - }
255   -
  210 + // see SI-5313
  211 + // search for clobbers of this store if we aren't doing l1 = null
  212 + // this doesn't catch the second store in x=null;l1=x; but in practice this catches
  213 + // a lot of null stores very cheaply
  214 + if (idx == 0 || bb(idx - 1) != CONSTANT(Constant(null)))
  215 + findClobbers(l1, bb, idx + 1)
  216 +
256 217 case nw @ NEW(REFERENCE(sym)) =>
257 218 assert(nw.init ne null, "null new.init at: " + bb + ": " + idx + "(" + instr + ")")
258 219 worklist += findInstruction(bb, nw.init)
@@ -277,6 +238,67 @@ abstract class DeadCodeElimination extends SubComponent {
277 238 }
278 239 }
279 240 }
  241 +
  242 + /**
  243 + * Finds and marks all clobbers of the given local starting in the given
  244 + * basic block at the given index
  245 + *
  246 + * Storing to local variables of reference or array type may be indirectly
  247 + * observable because it may remove a reference to an object which may allow the object
  248 + * to be gc'd. See SI-5313. In this code I call the LOCAL_STORE(s) that immediately follow a
  249 + * LOCAL_STORE and that store to the same local "clobbers." If a LOCAL_STORE is marked
  250 + * useful then its clobbers must go into the set of clobbers, which will be
  251 + * compensated for later
  252 + */
  253 + def findClobbers(l: Local, bb: BasicBlock, idx: Int) {
  254 + // previously visited blocks tracked to prevent searching forever in a cycle
  255 + val inspected = mutable.Set[BasicBlock]()
  256 + // our worklist of blocks that still need to be checked
  257 + val blocksToBeInspected = mutable.Set[BasicBlock]()
  258 +
  259 + // Tries to find the next clobber of l1 in bb1 starting at idx1.
  260 + // if it finds one it adds the clobber to clobbers set for later
  261 + // handling. If not it adds the direct successor blocks to
  262 + // the uninspectedBlocks to try to find clobbers there. Either way
  263 + // it adds the exception successor blocks for further search
  264 + def findClobberInBlock(idx1: Int, bb1: BasicBlock) {
  265 + val key = ((l, bb1))
  266 + val foundClobber = (localStores contains key) && {
  267 + def minIdx(s : mutable.BitSet) = if(s.isEmpty) -1 else s.min
  268 +
  269 + // find the smallest index greater than or equal to idx1
  270 + val clobberIdx = minIdx(localStores(key) dropWhile (_ < idx1))
  271 + if (clobberIdx == -1)
  272 + false
  273 + else {
  274 + debuglog(s"\t${bb1(clobberIdx)} is a clobber of ${bb(idx)}")
  275 + clobbers += ((bb1, clobberIdx))
  276 + true
  277 + }
  278 + }
  279 +
  280 + // always need to look into the exception successors for additional clobbers
  281 + // because we don't know when flow might enter an exception handler
  282 + blocksToBeInspected ++= (bb1.exceptionSuccessors filterNot inspected)
  283 + // If we didn't find a clobber here then we need to look at successor blocks.
  284 + // if we found a clobber then we don't need to search in the direct successors
  285 + if (!foundClobber) {
  286 + blocksToBeInspected ++= (bb1.directSuccessors filterNot inspected)
  287 + }
  288 + }
  289 +
  290 + // first search starting at the current index
  291 + // note we don't put bb in the inspected list yet because a loop may later force
  292 + // us back around to search from the beginning of bb
  293 + findClobberInBlock(idx, bb)
  294 + // then loop until we've exhausted the set of uninspected blocks
  295 + while(!blocksToBeInspected.isEmpty) {
  296 + val bb1 = blocksToBeInspected.head
  297 + blocksToBeInspected -= bb1
  298 + inspected += bb1
  299 + findClobberInBlock(0, bb1)
  300 + }
  301 + }
280 302
281 303 def sweep(m: IMethod) {
282 304 val compensations = computeCompensations(m)
@@ -306,6 +328,12 @@ abstract class DeadCodeElimination extends SubComponent {
306 328 i match {
307 329 case NEW(REFERENCE(sym)) =>
308 330 log(s"Eliminated instantation of $sym inside $m")
  331 + case STORE_LOCAL(l) if clobbers contains ((bb, idx)) =>
  332 + // if an unused instruction was a clobber of a used store to a reference or array type
  333 + // then we'll replace it with the store of a null to make sure the reference is
  334 + // eliminated. See SI-5313
  335 + bb emit CONSTANT(Constant(null))
  336 + bb emit STORE_LOCAL(l)
309 337 case _ => ()
310 338 }
311 339 debuglog("Skipped: bb_" + bb + ": " + idx + "( " + i + ")")
2  test/files/run/t5313.check
@@ -6,3 +6,5 @@ STORE_LOCAL(value kept3)
6 6 STORE_LOCAL(variable kept2)
7 7 STORE_LOCAL(variable kept4)
8 8 STORE_LOCAL(variable kept4)
  9 +STORE_LOCAL(variable kept5)
  10 +STORE_LOCAL(variable kept5)
8 test/files/run/t5313.scala
@@ -13,6 +13,7 @@ object Test extends IcodeTest {
13 13 val result = new java.lang.ref.WeakReference(kept1)
14 14 kept1 = null // we can't eliminate this assigment because result can observe
15 15 // when the object has no more references. See SI-5313
  16 + kept1 = new Object // but we can eliminate this one because kept1 has already been clobbered
16 17 var erased2 = null // we can eliminate this store because it's never used
17 18 val erased3 = erased2 // and this
18 19 var erased4 = erased2 // and this
@@ -20,7 +21,7 @@ object Test extends IcodeTest {
20 21 var kept2: Object = new Object // ultimately can't be eliminated
21 22 while(foo) {
22 23 val kept3 = kept2
23   - kept2 = null // this can't, because it clobbers x, which is ultimately used
  24 + kept2 = null // this can't, because it clobbers kept2, which is used
24 25 erased4 = null // safe to eliminate
25 26 println(kept3)
26 27 }
@@ -30,6 +31,11 @@ object Test extends IcodeTest {
30 31 catch {
31 32 case _ : Throwable => kept4 = null // have to keep, it clobbers kept4 which is used
32 33 }
  34 + var kept5 = new Object
  35 + print(kept5)
  36 + kept5 = null // can't eliminate it's a clobber and it's used
  37 + print(kept5)
  38 + kept5 = null // can eliminate because we don't care about clobbers of nulls
33 39 result
34 40 }
35 41 }""".stripMargin

0 comments on commit 9b4fa83

Please sign in to comment.
Something went wrong with that request. Please try again.