On Fri, Jan 20, 2017 at 3:30 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
> On Thu, Jan 19, 2017 at 12:07 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>> On Thu, Jan 19, 2017 at 11:22 AM, Richard Biener
>> <richard.guent...@gmail.com> wrote:
>>> On Thu, Jan 19, 2017 at 11:25 AM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>>>> On Thu, Jan 19, 2017 at 9:42 AM, Richard Biener
>>>
>>> Or a later pass introduced it (in this case, the vectorizer).
>>>
>>>> Predcom could be the
>>>> only pass that can handle such case as it understands data reference
>>>> better. Note Martin's patch is not to skip handling of length == 0
>>>> chain, later ref will still be CSEed with result of root ref, only the
>>>> combination operation like chain1 + chain2 is skipped. In this case,
>>>> following dom should be able to handle such (loop independent) cse
>>>> opportunities.
>>>
>>> I must admit I don't completely understand the consequences of this
>>> disabling but of course DOM should also be able to handle the CSE
>>> (ok, DOM is known to be quite weak with memory equivalence but
>>> it's not that data-dependence is really better in all cases).
>>>
>>> Can't we simply re-order refs in new_chain appropriately or handle
>>> this case in combinable_refs_p instead?
>> It's not refs need to be reordered, root ref always dominates others.
>> But yes, we need to find a dominator insertion place for combined
>> operation. Looking at function reassociate_to_the_same_stmt, it
>> simply inserts new_stmt at root_stmt of root ref, which causes ICE in
>> this case. The new_stmt needs to be inserted at a place also
>> dominating combination of later refs. We can either compute the
>> information in place, or compute and pass the information from
>> previous combinable_refs_p. This should be the real fix.
> Hi All,
> As analyzed, root cause is Predcom inserts combined stmt at place only
> wrto the root refs. This is not enough because after combination,
> result is also used by the following combined refs, not only root
> refs. This patch fixes the ICE by processing all refs reversely and
> computing dominance point for insertion. When it comes to the root
> refs, dominance point is ready for use.
> Bootstrap and test on x86_64 and AArch64 ongoing, is it OK if no failures?
Ok.
RIchard.
> Thanks,
> bin
> 2017-01-19 Bin Cheng <bin.ch...@arm.com>
>
> PR tree-optimization/70754
> * tree-predcom.c (stmt_combining_refs): New parameter INSERT_BEFORE.
> (reassociate_to_the_same_stmt): New parameter INSERT_BEFORE. Insert
> combined stmt before it if not NULL.
> (combine_chains): Process refs reversely and compute dominance point
> for root ref.
>
> gcc/testsuite/ChangeLog
> 2017-01-19 Bin Cheng <bin.ch...@arm.com>
>
> PR tree-optimization/70754
> * gfortran.dg/pr70754.f90: New test.
>
>>> That is, I understand the patch as a hack as it should be always
>>> possible to find dominating refs?
>>>
>>> In fact the point of the assert tells us a simpler fix may be
>>>
>>> Index: tree-predcom.c
>>> ===================================================================
>>> --- tree-predcom.c (revision 244519)
>>> +++ tree-predcom.c (working copy)
>>> @@ -2330,6 +2334,12 @@ combine_chains (chain_p ch1, chain_p ch2
>>> break;
>>> }
>>> }
>>> + if (new_chain->length == 0
>>> + && ! new_chain->has_max_use_after)
>>> + {
>>> + release_chain (new_chain);
>>> + return NULL;
>>> + }
>>>
>>> ch1->combined = true;
>>> ch2->combined = true;
>>>
>>> which obviously matches the assert we run into for the testcase? I'd
>>> be ok with that
>>> (no stmt_dominates_stmt_p, heh) with a suitable comment before it.
>>>
>>> Richard.
>>>
