This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: LRA: check_rtl modifies RTL instruction stream


On 11/8/2013, 9:13 AM, Robert Suchanek wrote:
Hi Vladimir,

I have been looking into regression testing for mips16 with LRA enabled
and tried to understand and solve some ICEs. It was found that in
a narrowed testcase (attached below) that there are two issues:

1. In the back end - pattern not recognized and hence ICE.
2. In the LRA - a bug that exposes the problem above.

The problem within LRA points to check_rtl function. The function
does not only check for the consistency of the instruction stream, but
unfortunately, it accidentally modifies it as well.

The fragment of the RTL dump before check_rtl():

(insn 18 7 12 2 (set (reg/f:DI 197)
        		      (symbol_ref:DI ("a")  <var_decl 0x7fd34f3af558 a>)) fpr-moves-7.c:7 280 {*movdi_64bit_mips16}
      (expr_list:REG_EQUIV (symbol_ref:DI ("a")  <var_decl 0x7fd34f3af558 a>)
         (nil)))

After check_rtl(), movdi_64bit_mips16 turns into *lea64:

(insn 18 7 12 2 (parallel [
             (set (reg/f:DI 197)
                 (symbol_ref:DI ("a")  <var_decl 0x7fd34f3af558 a>))
             (clobber (scratch:DI))
         ]) fpr-moves-7.c:7 258 {*lea64}
      (expr_list:REG_EQUIV (symbol_ref:DI ("a")  <var_decl 0x7fd34f3af558 a>)
         (nil)))

What happens here is that check_rtl calls insn_invalid_p and insn_invalid_p
tries to add clobber registers in the hope to match a pattern. In our case,
adding a clobber does match *lea64 and insn_invalid_p generates new
instruction. The reason for doing this is that reload_in_progress is not being set
when LRA is running. Otherwise, insn_invalid_p would be prevented to add clobbers.
The problem does not exist if we run it with the classic reload.

One of the solutions I can think of is adding !lra_in_progress to insn_invalid_p
and set this variable before check_rtl() but I am not fully confident that this
is so trivial (I am new to the gcc hacking business). I see a number of reasons
that reload_in_progress is not being used when LRA is used, thus, not entirely
sure if this change would not break anything else.
No, you are perfectly right. At the end of LRA all insns should be valid *without any change*. That is the purpose of check_rtl. There are a lot of reasons why we use lra_in_progress and not reload_in_progress. Unfortunately, there are too many places where reload_in_progress is and was used and apparently one of this place was overlooked.

So adding !lra_in_progress along with !reload_in_progress is a right solution here.
Can you suggest how to guarantee check_rtl does not modify the insns?
The back end issue will be looked separately by us.

Thanks for finding this.  I guess you can submit the patch.

Please use also gcc-patches mailing lists for such discussions. It is a better and right place for this.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]