On Mon, Apr 25, 2016 at 8:27 AM, H.J. Lu <hjl.to...@gmail.com> wrote:
> On Mon, Apr 25, 2016 at 8:10 AM, Uros Bizjak <ubiz...@gmail.com> wrote:
>> On Mon, Apr 25, 2016 at 4:47 PM, H.J. Lu <hjl.to...@gmail.com> wrote:
>>> On Mon, Apr 25, 2016 at 7:18 AM, Uros Bizjak <ubiz...@gmail.com> wrote:
>>>> On Mon, Apr 25, 2016 at 2:51 PM, H.J. Lu <hongjiu...@intel.com> wrote:
>>>>> Tested on Linux/x86-64. OK for trunk?
>>>>
>>>>> + /* FIXME: Since the CSE pass may change dominance info, which isn't
>>>>> + expected by the fwprop pass, call free_dominance_info to
>>>>> + invalidate dominance info. Otherwise, the fwprop pass may crash
>>>>> + when dominance info is changed. */
>>>>> + if (TARGET_64BIT)
>>>>> + free_dominance_info (CDI_DOMINATORS);
>>>>> +
>>>>
>>>> Please resolve the above problem first, target-dependent sources are
>>>> not the place to apply band-aids for middle-end problems. The thread
>>>> with the proposed fix died in [1].
>>>>
>>>> [1] https://gcc.gnu.org/ml/gcc/2016-03/msg00143.html
>>>
>>> free_dominance_info (CDI_DOMINATORS) has been called in other
>>> places to avoid this middle-end issue. I don't know when the middle-end
>>> will be fixed. I don't think this target optimization should be penalized
>>> by
>>> the middle-end issue.
>>
>> Let's ask Richard if he is OK with the workaround...
>>
>> One more thing:
>>
>> @@ -3551,6 +3874,7 @@ convert_scalars_to_vector ()
>> basic_block bb;
>> bitmap candidates;
>> int converted_insns = 0;
>> + rtx zero, minus_one;
>>
>> bitmap_obstack_initialize (NULL);
>> candidates = BITMAP_ALLOC (NULL);
>> @@ -3585,22 +3909,40 @@ convert_scalars_to_vector ()
>> if (dump_file)
>> fprintf (dump_file, "There are no candidates for optimization.\n");
>>
>> + if (TARGET_64BIT)
>> + {
>> + zero = gen_reg_rtx (V1TImode);
>> + minus_one = gen_reg_rtx (V1TImode);
>> + }
>> + else
>> + {
>> + zero = NULL_RTX;
>> + minus_one = NULL_RTX;
>> + }
>> +
>>
>> Do we *really* need to crate registers here? They are only used as a
>> temporary in scalar_chain_64::convert_insn, and I think that any CSE
>> worth its name should find out when the same immediate is loaded to
>> different temporaries.
>
> I will double check. Last time when I tried, CSE didn't work on this for a
> reason. For
>
> [hjl@gnu-6 pr67400]$ cat z.i
> extern void bar (void);
>
> void *
> foo (void)
> {
> return &bar;
> }
> [hjl@gnu-6 pr67400]$ gcc -S -fPIC -O2 z.i
> [hjl@gnu-6 pr67400]$
>
> CSE sees:
>
> (insn 5 2 6 2 (set (reg:DI 89)
> (mem/u/c:DI (const:DI (unspec:DI [
> (symbol_ref:DI ("bar") [flags 0x41]
> <function_decl 0x7f24362151b0 bar>)
> ] UNSPEC_GOTPCREL)) [1 S8 A8])) z.i:6 89
> {*movdi_internal}
> (nil))
> (insn 6 5 10 2 (set (reg:DI 87 [ <retval> ])
> (reg:DI 89)) z.i:6 89 {*movdi_internal}
> (expr_list:REG_EQUAL (symbol_ref:DI ("bar") [flags 0x41]
> <function_decl 0x7f24362151b0 bar>)
> (nil)))
>
> CSE will not change it to
>
> (insn 6 5 10 2 (set (reg:DI 89 [ <retval> ])
> (reg:DI 89)) z.i:6 89 {*movdi_internal}
> (expr_list:REG_EQUAL (symbol_ref:DI ("bar") [flags 0x41]
> <function_decl 0x7f24362151b0 bar>)
> (nil)))
I meant CSE won't change it to
insn 5 2 9 2 (set (reg:DI 87 [ <retval> ])
(symbol_ref:DI ("bar") [flags 0x41] <function_decl
0x7f7a734971b0 bar>)) z.i:6 89 {*movdi_internal}
(nil))
>> BTW: I'd really like if Ilya can review the functionality of the
>> patch, he is an expert in this conversion stuff.
>>
>> Uros.
>
> Ilya, can you take a look?
>
> Thanks.
>
> --
> H.J.
--
H.J.
