Re: [PATCH GCC8][33/33]Fix PR69710/PR68030 by reassociate vect base address and a simple CSE pass

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Mon, Jul 3, 2017 at 5:12 PM, Jeff Law <law@redhat.com> wrote:
> On 04/18/2017 04:54 AM, Bin Cheng wrote:
>> Hi,
>> This is the same patch posted at https://gcc.gnu.org/ml/gcc-patches/2016-05/msg02000.html,
>> after rebase against this patch series.  This patch was blocked because without this patch
>> series, it could generate worse code on targets with limited addressing mode support, like
>> AArch64.
>> There was some discussion about alternative fix for PRs, but after thinking twice I think
>> this fix is in the correct direction.  A CSE interface is useful to clean up code generated
>> in vectorizer, and we should improve this CSE interface into a region base one.  for the
>> moment, optimal code is not generated on targets like x86, I believe it's because the CSE
>> is weak and doesn't cover all basic blocks generated by vectorizer, the issue should be
>> fixed if region-based CSE is implemented.
>> Is it OK?
>>
>> Thanks,
>> bin
>> 2017-04-11  Bin Cheng  <bin.cheng@arm.com>
>>
>>       PR tree-optimization/68030
>>       PR tree-optimization/69710
>>       * tree-ssa-dom.c (cse_bbs): New function.
>>       * tree-ssa-dom.h (cse_bbs): New declaration.
>>       * tree-vect-data-refs.c (vect_create_addr_base_for_vector_ref):
>>       Re-associate address by splitting constant offset.
>>       (vect_create_data_ref_ptr, vect_setup_realignment): Record changed
>>       basic block.
>>       * tree-vect-loop-manip.c (vect_gen_prolog_loop_niters): Record
>>       changed basic block.
>>       * tree-vectorizer.c (tree-ssa-dom.h): Include header file.
>>       (changed_bbs): New variable.
>>       (vectorize_loops): Allocate and free CHANGED_BBS.  Call cse_bbs.
>>       * tree-vectorizer.h (changed_bbs): New declaration.
>>
> So are you still interested in moving this forward Bin?  I know you did
> a minor update in response to Michael Meissner's problems.  Is there
> another update planned?
Hi,
Yes I want to move this forward, but better with confirmation whether
the regression is real or just u-arch hazard.  OTOH, given the
proceeding 32 patches have been applied, I think there is no need to
hold this one.  It is proceeding patch rather than this one causes
regression I guess.

>
> THe only obvious thing I'd suggest changing in the DOM interface is to
> have continue to walk the dominator tree, but do nothing for blocks that
> are not in changed_bbs.  That way you walk blocks in changed_bbs in
> dominator order rather than in bb->index order.
I will update patch as you suggested, but may take some time.

Thanks,
bin
>
> Jeff
>