RVM
  1. RVM
  2. RVM-16

Constant folding values from TIBs produces failures

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.2
    • Component/s: Compiler: Optimizing
    • Labels:
      None
    • Number of attachments :
      1

      Description

      Constant folding of values in TIBs (the option CF_TIB in OPT_Simplifier) broke regression tests in the past. This should be re-evaluated and fixed.

        Issue Links

          Activity

          Hide
          Ian Rogers added a comment -

          Having spent yesterday distracted thinking I had my own bug, I found I was actually looking at this bug I have a concrete example of the problem in that we simplify:

          3 get_obj_tib t913sa([Ljava/lang/Object = t528psa(Ljava/util/Collection;,p), <TRUEGUARD>

          into

          3 ref_move t913sa([Ljava/lang/Object = tib "java.util.Collection"

          However, a Collection is an interface so it's TIB won't really be the TIB we're after. What has confused the simplifier is that the register is marked as having a precise type. This should mean the value of the register was created by a new. But a collection is an interface which you can't possibly allocate or have as precise!

          The bug occurs in java.security.CodeSource.<init> which inlines the Collection version of HashMaps constructor (inlining must be enabled for the bug to occur ). The culprit of incorrectly specifying the precise type on the register appears to be OPT_BranchOptimizations.

          I'd suggest that we:
          1) disable folding get_obj_tib on registers (constants should be ok)
          2) create an IR verifier that when we check register operands of instructions, if a register operand is marked as precise it can't be an interface or abstract class
          3) fix all the places this shows the precise type information has become corrupt

          which is quite a bit of work, but it's better we're strict about register properties prior to re-introducing all the O3 optimization phases (that probably violate them as well). This won't stop all possible corruptions of register precise type information (corruptions from a class to another class will be possible), but hopefully we have enough test cases that we will find all our bugs.

          Show
          Ian Rogers added a comment - Having spent yesterday distracted thinking I had my own bug, I found I was actually looking at this bug I have a concrete example of the problem in that we simplify: 3 get_obj_tib t913sa([Ljava/lang/Object = t528psa(Ljava/util/Collection;,p), <TRUEGUARD> into 3 ref_move t913sa([Ljava/lang/Object = tib "java.util.Collection" However, a Collection is an interface so it's TIB won't really be the TIB we're after. What has confused the simplifier is that the register is marked as having a precise type. This should mean the value of the register was created by a new. But a collection is an interface which you can't possibly allocate or have as precise! The bug occurs in java.security.CodeSource.<init> which inlines the Collection version of HashMaps constructor (inlining must be enabled for the bug to occur ). The culprit of incorrectly specifying the precise type on the register appears to be OPT_BranchOptimizations. I'd suggest that we: 1) disable folding get_obj_tib on registers (constants should be ok) 2) create an IR verifier that when we check register operands of instructions, if a register operand is marked as precise it can't be an interface or abstract class 3) fix all the places this shows the precise type information has become corrupt which is quite a bit of work, but it's better we're strict about register properties prior to re-introducing all the O3 optimization phases (that probably violate them as well). This won't stop all possible corruptions of register precise type information (corruptions from a class to another class will be possible), but hopefully we have enough test cases that we will find all our bugs.
          Hide
          Ian Rogers added a comment -

          Phase 1 is committed as r12970. Rather than use the verifier it's easier to track the bug via asserts. Adding asserts to ensure precise type information is preserved is what I'm doing currently.

          Show
          Ian Rogers added a comment - Phase 1 is committed as r12970. Rather than use the verifier it's easier to track the bug via asserts. Adding asserts to ensure precise type information is preserved is what I'm doing currently.
          Hide
          Ian Rogers added a comment -

          Attached is the state of play on this. It seems the problem is that a checkcast is setting the type of a register operand to whatever it's checking even though the register operand is precise. This can weaken the strong type information meaning we have some specific type of collection but then following a cast it's now just a collection. The precise type information is wrong and this has the potential not just to break the simplifier but also inlining. This patch does 3 things:

          1) it makes all accesses to a register operands register and type go through a getter and setter (praise be to eclipse's refactor tool)
          2) it adds asserts to trap when precise information is given which is wrong (namely the precise type is abstract or an interface). So that this doesn't blow up some statements setting a precise type needed reordering.
          3) it removes the java.lang.String special case from the field database (shouldn't be necessary since we have the boot image writer oracle now) and ignores the setting of the type following a checkcast if the type is already precise

          It seems likely that the special case is still required for the field database Or the checkcast change is improving inlining and exposing latent bugs. In any case dacapo bloat is now failing (with an error relating to a string). It seems the string special case should be re-introduced or the checkcast modified so that it clears the precise type when weakening what we know about it. After this the get_obj_tib optimization can be tried out again.

          I'm not going to get chance to play with this for a while so if someone else wants a go feel free.

          Show
          Ian Rogers added a comment - Attached is the state of play on this. It seems the problem is that a checkcast is setting the type of a register operand to whatever it's checking even though the register operand is precise. This can weaken the strong type information meaning we have some specific type of collection but then following a cast it's now just a collection. The precise type information is wrong and this has the potential not just to break the simplifier but also inlining. This patch does 3 things: 1) it makes all accesses to a register operands register and type go through a getter and setter (praise be to eclipse's refactor tool) 2) it adds asserts to trap when precise information is given which is wrong (namely the precise type is abstract or an interface). So that this doesn't blow up some statements setting a precise type needed reordering. 3) it removes the java.lang.String special case from the field database (shouldn't be necessary since we have the boot image writer oracle now) and ignores the setting of the type following a checkcast if the type is already precise It seems likely that the special case is still required for the field database Or the checkcast change is improving inlining and exposing latent bugs. In any case dacapo bloat is now failing (with an error relating to a string). It seems the string special case should be re-introduced or the checkcast modified so that it clears the precise type when weakening what we know about it. After this the get_obj_tib optimization can be tried out again. I'm not going to get chance to play with this for a while so if someone else wants a go feel free.
          Hide
          Ian Rogers added a comment -

          This should be fixed as of r13007 (see also r13004 for the problems that were gating this fix). Not closing until we have a clean set of sanity runs, but passed pre-commit and commit tests that include the previously failing SPECjbb benchmarks.

          Show
          Ian Rogers added a comment - This should be fixed as of r13007 (see also r13004 for the problems that were gating this fix). Not closing until we have a clean set of sanity runs, but passed pre-commit and commit tests that include the previously failing SPECjbb benchmarks.
          Hide
          Ian Rogers added a comment -

          Last nights regression results [1] don't show new opt compiler failures so I'm closing.
          [1] http://jikesrvm.anu.edu.au/cattrack/results/rvmx86lnx32.anu.edu.au/sanity.546/regression_report

          Show
          Ian Rogers added a comment - Last nights regression results [1] don't show new opt compiler failures so I'm closing. [1] http://jikesrvm.anu.edu.au/cattrack/results/rvmx86lnx32.anu.edu.au/sanity.546/regression_report

            People

            • Assignee:
              Ian Rogers
              Reporter:
              Peter Donald
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: