On 10/16/18 10:10 AM, Ilya Leoshkevich wrote:
>> Am 16.10.2018 um 16:20 schrieb Jeff Law <l...@redhat.com>:
>>
>> On 10/16/18 3:17 AM, Ilya Leoshkevich wrote:
>>> Bootstrap and regtest are running on x86_64-redhat-linux.
>>>
>>> Insns in FROM..TO range might not be recognized yet, and their
>>> corresponding entries in lra_insn_recog_data[] might be NULLs. In the
>>> code from PR87596 the problematic insn is merely
>>>
>>> (note 148 154 68 7 NOTE_INSN_DELETED)
>>>
>>> however I assume that non-note insns may occur as well. So make sure
>>> they are recognized before touching their lra_insn_recog_data_t.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2018-10-16 Ilya Leoshkevich <i...@linux.ibm.com>
>>>
>>> PR rtl-optimization/87596
>>> * lra-constraints.c (spill_hard_reg_in_range): Use
>>> lra_get_insn_recog_data () instead of lra_insn_recog_data[]
>>> for instructions in FROM..TO range.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2018-10-16 Ilya Leoshkevich <i...@linux.ibm.com>
>>>
>>> PR rtl-optimization/87596
>>> * gcc.target/i386/pr87596.c: New test.
>> I think this is just papering over the problem.
>>
>> AFAICT for something like a deleted note lra_get_insn_recog_data is
>> going to call lra_set_insn_recog_data. Given that INSN will be a NOTE I
>> think icode will end up being -1 and we'll eventually get into this clause:
>>
>> if (icode < 0)
>> {
>> int nop, nalt;
>> machine_mode operand_mode[MAX_RECOG_OPERANDS];
>> const char *constraints[MAX_RECOG_OPERANDS];
>>
>> nop = asm_noperands (PATTERN (insn));
>>
>> And I don't think we should be trying to extract the PATTERN of a note.
>>
>> I think you need to either filter out the non-insn thingies in
>> spill_hard_reg_in_range, or go with your patch plus beefing up the
>> checks in lra_set_insn_recog_data.
>>
>> jeff
>>
>
> Thanks, you’re absolutely right!
>
> I re-ran the test with --enable-checking=all, and it crashed in another
> branch of that if statement:
>
> else
> {
> insn_extract (insn); /* Also uses PATTERN (). */
> data->insn_static_data = insn_static_data
>
> I looked at the usage patterns of lra_insn_recog_data[] and
> lra_get_insn_recog_data (), and noticed that we use the former directly
> only when the insn in question is taken from insn_bitmap, and the latter
> in all other cases. Furthermore, lra_get_insn_recog_data () is normally
> guarded by INSN_P () or something similar.
>
> So I plan to keep using lra_get_insn_recog_data (), but with an extra
> INSN_P () check.
Seems reasonable. Though note that INSN_P will accept DEBUG_INSNs.
That may be OK -- those routines have some code for handling
DEBUG_INSNs, but I didn't audit those paths carefully.
Jeff
