On Mon, Jan 5, 2015 at 2:12 PM, Martin Liška <mli...@suse.cz> wrote:
> On 12/19/2014 12:04 PM, Richard Biener wrote:
>>
>> On Thu, Dec 18, 2014 at 6:38 PM, Martin Liška <mli...@suse.cz> wrote:
>>>
>>> On 12/17/2014 04:23 PM, Richard Biener wrote:
>>>>
>>>>
>>>> On Wed, Dec 17, 2014 at 12:17 PM, Martin Liška <mli...@suse.cz> wrote:
>>>>>
>>>>>
>>>>> On 12/11/2014 01:37 PM, Richard Biener wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mli...@suse.cz> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hello.
>>>>>>>
>>>>>>> As suggested by Richard, I split compare_operand functions to various
>>>>>>> functions
>>>>>>> related to a specific comparison. Apart from that I added fast check
>>>>>>> for
>>>>>>> volatility flag that caused miscompilation mentioned in PR63569.
>>>>>>>
>>>>>>> Patch can bootstrap on x86_64-linux-pc without any regression seen
>>>>>>> and
>>>>>>> I
>>>>>>> was
>>>>>>> able to build Firefox with LTO.
>>>>>>>
>>>>>>> Ready for trunk?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hmm, I don't think the dispatch to compare_memory_operand is at the
>>>>>> correct place. It should be called from places where currently
>>>>>> compare_operand is called and it should recurse to compare_operand.
>>>>>> That is, it is more "high-level".
>>>>>>
>>>>>> Can you please fix the volatile issue separately? It's also not
>>>>>> necessary
>>>>>> to do that check on every operand but just on memory operands.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Hello Richard.
>>>>>
>>>>> I hope the following patch is the finally the right approach we want to
>>>>> do
>>>>> ;)
>>>>> In the first patch, I did just refactoring for high-level memory
>>>>> operand
>>>>> comparison and the second of fixes problem connected to missing
>>>>> volatile
>>>>> attribute comparison.
>>>>>
>>>>> Patch was retested and can bootstrap.
>>>>>
>>>>> What do you think about it?
>>>>
>>>>
>>>>
>>>> +bool
>>>> +func_checker::compare_cst_or_decl (tree t1, tree t2)
>>>> +{
>>>> ...
>>>> + case INTEGER_CST:
>>>> + {
>>>> + ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
>>>> + && wi::to_offset (t1) == wi::to_offset (t2);
>>>> +
>>>> + return return_with_debug (ret);
>>>> + }
>>>> + case COMPLEX_CST:
>>>> + case VECTOR_CST:
>>>> + case STRING_CST:
>>>> + case REAL_CST:
>>>> + {
>>>> + ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
>>>> + return return_with_debug (ret);
>>>>
>>>> why does the type matter for INTEGER_CSTs but not for other constants?
>>>> Also comparing INTEGER_CSTs via to_offset is certainly wrong. Please
>>>> use tree_int_cst_equal_p (t1, t2) instead, or operand_equal_p which
>>>> would
>>>> end up calling tree_int_cst_equal_p. That said, I'd have commonized all
>>>> _CST nodes by
>>>>
>>>> ret = compatible_types_p (....) && operand_equal_p (...);
>>>>
>>>> + case CONST_DECL:
>>>> + case BIT_FIELD_REF:
>>>> + {
>>>>
>>>> I'm quite sure BIT_FIELD_REF is spurious here.
>>>>
>>>> Now to the "meat" ...
>>>>
>>>> + tree load1 = get_base_loadstore (t1, false);
>>>> + tree load2 = get_base_loadstore (t2, false);
>>>> +
>>>> + if (load1 && load2 && !compare_memory_operand (t1, t2))
>>>> + return return_false_with_msg ("memory operands are different");
>>>> + else if ((load1 && !load2) || (!load1 && load2))
>>>> + return return_false_with_msg ("");
>>>> +
>>>>
>>>> and the similar code for assigns. The way you introduce the
>>>> unpack_handled_component flag to get_base_loadstore makes
>>>> it treat x.field as non-memory operand. That's wrong. I wonder
>>>> why you think you needed that. Why does
>>>>
>>>> tree load1= get_base_loadstore (t1);
>>>>
>>>> not work? Btw, I'd have avoided get_base_loadstore and simply did
>>>>
>>>> ao_ref_init (&r1, t1);
>>>> ao_ref_init (&r2, t2);
>>>> tree base1 = ao_ref_base (&r1);
>>>> tree base2 = ao_ref_base (&r2);
>>>> if ((DECL_P (base1) || (TREE_CODE (base1) == MEM_REF || TREE_CODE
>>>> (base1) == TARGET_MEM_REF))
>>>> && (DECL_P (base2) || (...))
>>>> ...
>>>>
>>>> or rather put this into compare_memory_operand and call that on all
>>>> operands
>>>> that may be memory (TREE_CODE () != SSA_NAME && !is_gimple_min_invariant
>>>> ()).
>>>>
>>>> I also miss where you handle memory operands on the LHS for calls
>>>> and for assignments.
>>>>
>>>> The compare_ssa_name refactoring part is ok to install.
>>>>
>>>> Can you please fix the volatile bug before the refactoring as it looks
>>>> like
>>>> we're going to iterate some more here unless I can find the time to give
>>>> it a quick try myself.
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>
>>> Hi.
>>>
>>> There's second part of the patch set.
>>
>>
>> It still has similar issues:
>>
>> +/* Function compare for equality given memory operands T1 and T2. */
>> +
>> +bool
>> +func_checker::compare_memory_operand (tree t1, tree t2)
>> +{
>> + if (!t1 && !t2)
>> + return true;
>> + else if (!t1 || !t2)
>> + return false;
>> +
>> + if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
>> + return return_false_with_msg ("different operand volatility");
>> +
>> + bool source_is_memop = DECL_P (t1) || INDIRECT_REF_P (t1)
>> + || TREE_CODE (t1) == MEM_REF
>> + || TREE_CODE (t1) == TARGET_MEM_REF;
>> +
>> + bool target_is_memop = DECL_P (t2) || INDIRECT_REF_P (t2)
>> + || TREE_CODE (t2) == MEM_REF
>> + || TREE_CODE (t2) == TARGET_MEM_REF;
>> +
>> + /* Compare alias sets for memory operands. */
>> + if (source_is_memop && target_is_memop)
>> + {
>> + ao_ref r1, r2;
>> + ao_ref_init (&r1, t1);
>> + ao_ref_init (&r2, t2);
>>
>> the *_is_memop tests need to work on the memory reference base. Thus
>> simply move the ao_ref_init's before those checks and do them on
>> the result of ao_ref_base ().
>>
>> Similarly the volatile check can be put into the if (source_is_memop
>> && target_is_memop) block, volatileness doesn't matter for non-memory
>> operands.
>>
>> Ok with that changes.
>>
>> Thanks,
>> Richard.
>
>
> Hello Richard.
>
> Just for being sure, there's final version of the patch that passes
> regression tests.
>
> Ready for trunk in this shape?
Ok.
Thanks,
Richard.
> Thanks,
> Martin
>
