| Message ID | VI1PR0802MB21762C390F041B8A1E2E3D43E7190@VI1PR0802MB2176.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
I was attempting to build a spec 2006 run on a little endian power8 PowerPC
system with all of your patches installed. I had problems with the dealII
benchmark:
--> ./xgcc -B./ -O2 -S function_lib.ii -ftree-vectorize
function_lib.cc: In member function ‘Tensor<2, dim> Functions::CosineFunction<dim>::hessian(const Point<dim>&, unsigned int) const [with int dim = 3]’:
function_lib.cc:518:3: error: RESULT_DECL should be read only when DECL_BY_REFERENCE is set
while verifying SSA_NAME result_43 in statement
result_43 = result_68 + 16;
function_lib.cc:518:3: internal compiler error: verify_ssa failed
0x10e1535b verify_ssa(bool, bool)
/home/meissner/fsf-src/ivopts/gcc/tree-ssa.c:1184
0x10a4f9db execute_function_todo
/home/meissner/fsf-src/ivopts/gcc/passes.c:1973
0x10a506d3 do_per_function
/home/meissner/fsf-src/ivopts/gcc/passes.c:1650
0x10a50953 execute_todo
/home/meissner/fsf-src/ivopts/gcc/passes.c:2016
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
I'll attach a bzip2'ed version of the function_lib.ii pre-processed file that I
used to this mail message.
I'll run the other benchmarks and compare it to the baseline version to see
what improvements there are.
By the way, if anybody else wanted to try the patches, but did not want to apply all 33 patches, I cloned the trunk and applied the patches to the following branch: svn+ssh://gcc.gnu.org/svn/gcc/branches/ibm/ivopts
I did a bootstrap and make check-{gcc,c++,fortran,lto} comparing the results to
the baseline (subversion id 246975).
There were 2 differences:
The baseline failed on gcc.dg/sms-4.c but succeeded on gcc.dg/sms-1.c.
Here are the sms-[14] lines from the baseline:
Executing on host: /home/meissner/fsf-build-ppc64le/trunk-246975/gcc/xgcc -B/home/meissner/fsf-build-ppc64le/trunk-246975/gcc/ /home/meissner/fsf-src/trunk-246975/gcc/testsuite/gcc.dg/sms-4.c -fno-diagnostics-show-caret
-fdiagnostics-color=never -O2 -fmodulo-sched -fmodulo-sched-allow-regmoves -fdump-rtl-sms --param sms-min-sc=1 -ffat-lto-objects -lm -o ./sms-4.exe (timeout = 300)
spawn /home/meissner/fsf-build-ppc64le/trunk-246975/gcc/xgcc -B/home/meissner/fsf-build-ppc64le/trunk-246975/gcc/ /home/meissner/fsf-src/trunk-246975/gcc/testsuite/gcc.dg/sms-4.c -fno-diagnostics-show-caret -fdiagnostics
-color=never -O2 -fmodulo-sched -fmodulo-sched-allow-regmoves -fdump-rtl-sms --param sms-min-sc=1 -ffat-lto-objects -lm -o ./sms-4.exe
PASS: gcc.dg/sms-4.c (test for excess errors)
Setting LD_LIBRARY_PATH to :/home/meissner/fsf-build-ppc64le/trunk-246975/gcc:/home/meissner/fsf-build-ppc64le/trunk-246975/powerpc64le-unknown-linux-gnu/./libatomic/.libs::/home/meissner/fsf-build-ppc64le/trunk-246975/g
cc:/home/meissner/fsf-build-ppc64le/trunk-246975/powerpc64le-unknown-linux-gnu/./libatomic/.libs:
spawn [open ...]
PASS: gcc.dg/sms-4.c execution test
FAIL: gcc.dg/sms-4.c scan-rtl-dump-times sms "SMS succeeded" 1
Executing on host: /home/meissner/fsf-build-ppc64le/trunk-246975/gcc/xgcc -B/home/meissner/fsf-build-ppc64le/trunk-246975/gcc/ /home/meissner/fsf-src/trunk-246975/gcc/testsuite/gcc.dg/sms-1.c -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -fmodulo-sched -fmodulo-sched-allow-regmoves -fdump-rtl-sms -ffat-lto-objects -lm -o ./sms-1.exe (timeout = 300)
spawn /home/meissner/fsf-build-ppc64le/trunk-246975/gcc/xgcc -B/home/meissner/fsf-build-ppc64le/trunk-246975/gcc/ /home/meissner/fsf-src/trunk-246975/gcc/testsuite/gcc.dg/sms-1.c -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -fmodulo-sched -fmodulo-sched-allow-regmoves -fdump-rtl-sms -ffat-lto-objects -lm -o ./sms-1.exe
PASS: gcc.dg/sms-1.c (test for excess errors)
Setting LD_LIBRARY_PATH to :/home/meissner/fsf-build-ppc64le/trunk-246975/gcc:/home/meissner/fsf-build-ppc64le/trunk-246975/powerpc64le-unknown-linux-gnu/./libatomic/.libs::/home/meissner/fsf-build-ppc64le/trunk-246975/gcc:/home/meissner/fsf-build-ppc64le/trunk-246975/powerpc64le-unknown-linux-gnu/./libatomic/.libs:
spawn [open ...]
PASS: gcc.dg/sms-1.c execution test
PASS: gcc.dg/sms-1.c scan-rtl-dump-times sms "SMS succeeded" 1
While here are the lines from the ivopts run:
Executing on host: /home/meissner/fsf-build-ppc64le/ivopts/gcc/xgcc -B/home/meissner/fsf-build-ppc64le/ivopts/gcc/ /home/meissner/fsf-src/ivopts/gcc/testsuite/gcc.dg/sms-1.c -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -fmodulo-sched -fmodulo-sched-allow-regmoves -fdump-rtl-sms -ffat-lto-objects -lm -o ./sms-1.exe (timeout = 300)
spawn /home/meissner/fsf-build-ppc64le/ivopts/gcc/xgcc -B/home/meissner/fsf-build-ppc64le/ivopts/gcc/ /home/meissner/fsf-src/ivopts/gcc/testsuite/gcc.dg/sms-1.c -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -fmodulo-sched -fmodulo-sched-allow-regmoves -fdump-rtl-sms -ffat-lto-objects -lm -o ./sms-1.exe
PASS: gcc.dg/sms-1.c (test for excess errors)
Setting LD_LIBRARY_PATH to :/home/meissner/fsf-build-ppc64le/ivopts/gcc:/home/meissner/fsf-build-ppc64le/ivopts/powerpc64le-unknown-linux-gnu/./libatomic/.libs::/home/meissner/fsf-build-ppc64le/ivopts/gcc:/home/meissner/fsf-build-ppc64le/ivopts/powerpc64le-unknown-linux-gnu/./libatomic/.libs:
spawn [open ...]
PASS: gcc.dg/sms-1.c execution test
FAIL: gcc.dg/sms-1.c scan-rtl-dump-times sms "SMS succeeded" 1
Executing on host: /home/meissner/fsf-build-ppc64le/ivopts/gcc/xgcc -B/home/meissner/fsf-build-ppc64le/ivopts/gcc/ /home/meissner/fsf-src/ivopts/gcc/testsuite/gcc.dg/sms-4.c -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -fmodulo-sched -fmodulo-sched-allow-regmoves -fdump-rtl-sms --param sms-min-sc=1 -ffat-lto-objects -lm -o ./sms-4.exe (timeout = 300)
spawn /home/meissner/fsf-build-ppc64le/ivopts/gcc/xgcc -B/home/meissner/fsf-build-ppc64le/ivopts/gcc/ /home/meissner/fsf-src/ivopts/gcc/testsuite/gcc.dg/sms-4.c -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -fmodulo-sched -fmodulo-sched-allow-regmoves -fdump-rtl-sms --param sms-min-sc=1 -ffat-lto-objects -lm -o ./sms-4.exe
PASS: gcc.dg/sms-4.c (test for excess errors)
Setting LD_LIBRARY_PATH to :/home/meissner/fsf-build-ppc64le/ivopts/gcc:/home/meissner/fsf-build-ppc64le/ivopts/powerpc64le-unknown-linux-gnu/./libatomic/.libs::/home/meissner/fsf-build-ppc64le/ivopts/gcc:/home/meissner/fsf-build-ppc64le/ivopts/powerpc64le-unknown-linux-gnu/./libatomic/.libs:
spawn [open ...]
PASS: gcc.dg/sms-4.c execution test
PASS: gcc.dg/sms-4.c scan-rtl-dump-times sms "SMS succeeded" 1
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? 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. Jeff
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 >
diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c index d9e5942..6d74c07 100644 --- a/gcc/tree-ssa-dom.c +++ b/gcc/tree-ssa-dom.c @@ -1765,3 +1765,50 @@ optimize_stmt (basic_block bb, gimple_stmt_iterator si, } return retval; } + +/* A local CSE interface which runs CSE for basic blocks recorded in + CHANGED_BBS. */ + +void +cse_bbs (bitmap changed_bbs) +{ + unsigned index; + bitmap_iterator bi; + gimple_stmt_iterator gsi; + + hash_table<expr_elt_hasher> *avail_exprs; + class avail_exprs_stack *avail_exprs_stack; + class const_and_copies *const_and_copies; + + avail_exprs = new hash_table<expr_elt_hasher> (1024); + avail_exprs_stack = new class avail_exprs_stack (avail_exprs); + const_and_copies = new class const_and_copies (); + + threadedge_initialize_values (); + /* Push a marker on the stacks of local information so that we know how + far to unwind when we finalize this block. */ + avail_exprs_stack->push_marker (); + const_and_copies->push_marker (); + + EXECUTE_IF_SET_IN_BITMAP (changed_bbs, 0, index, bi) + { + basic_block bb = BASIC_BLOCK_FOR_FN (cfun, index); + + if (dump_file && (dump_flags & TDF_DETAILS)) + fprintf (dump_file, "\n\nRun local cse on block #%d\n\n", bb->index); + + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) + optimize_stmt (bb, gsi, const_and_copies, avail_exprs_stack); + + /* Pop stacks to keep it small. */ + avail_exprs_stack->pop_to_marker (); + const_and_copies->pop_to_marker (); + } + + delete avail_exprs; + avail_exprs = NULL; + + delete avail_exprs_stack; + delete const_and_copies; + threadedge_finalize_values (); +} diff --git a/gcc/tree-ssa-dom.h b/gcc/tree-ssa-dom.h index ad1b7ef..88869fd 100644 --- a/gcc/tree-ssa-dom.h +++ b/gcc/tree-ssa-dom.h @@ -24,5 +24,6 @@ extern bool simple_iv_increment_p (gimple *); extern void record_temporary_equivalences (edge, class const_and_copies *, class avail_exprs_stack *); +extern void cse_bbs (bitmap); #endif /* GCC_TREE_SSA_DOM_H */ diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c index aa504b6..beffa17 100644 --- a/gcc/tree-vect-data-refs.c +++ b/gcc/tree-vect-data-refs.c @@ -4247,23 +4247,27 @@ vect_create_addr_base_for_vector_ref (gimple *stmt, base_name = get_name (DR_REF (dr)); } - /* Create base_offset */ - base_offset = size_binop (PLUS_EXPR, - fold_convert (sizetype, base_offset), - fold_convert (sizetype, init)); + base_offset = fold_convert (sizetype, base_offset); + init = fold_convert (sizetype, init); if (offset) { offset = fold_build2 (MULT_EXPR, sizetype, fold_convert (sizetype, offset), step); - base_offset = fold_build2 (PLUS_EXPR, sizetype, - base_offset, offset); + if (TREE_CODE (offset) == INTEGER_CST) + init = fold_build2 (PLUS_EXPR, sizetype, init, offset); + else + base_offset = fold_build2 (PLUS_EXPR, sizetype, + base_offset, offset); } if (byte_offset) { byte_offset = fold_convert (sizetype, byte_offset); - base_offset = fold_build2 (PLUS_EXPR, sizetype, - base_offset, byte_offset); + if (TREE_CODE (byte_offset) == INTEGER_CST) + init = fold_build2 (PLUS_EXPR, sizetype, init, byte_offset); + else + base_offset = fold_build2 (PLUS_EXPR, sizetype, + base_offset, byte_offset); } /* base + base_offset */ @@ -4277,6 +4281,10 @@ vect_create_addr_base_for_vector_ref (gimple *stmt, } vect_ptr_type = build_pointer_type (STMT_VINFO_VECTYPE (stmt_info)); + addr_base = force_gimple_operand (addr_base, &seq, true, NULL); + gimple_seq_add_seq (new_stmt_list, seq); + /* Add constant offset at last. */ + addr_base = fold_build_pointer_plus (addr_base, init); dest = vect_get_new_vect_var (vect_ptr_type, vect_pointer_var, base_name); addr_base = force_gimple_operand (addr_base, &seq, true, dest); gimple_seq_add_seq (new_stmt_list, seq); @@ -4507,12 +4515,13 @@ vect_create_data_ref_ptr (gimple *stmt, tree aggr_type, struct loop *at_loop, if (new_stmt_list) { if (pe) - { - new_bb = gsi_insert_seq_on_edge_immediate (pe, new_stmt_list); - gcc_assert (!new_bb); - } + { + new_bb = gsi_insert_seq_on_edge_immediate (pe, new_stmt_list); + gcc_assert (!new_bb); + bitmap_set_bit (changed_bbs, pe->src->index); + } else - gsi_insert_seq_before (gsi, new_stmt_list, GSI_SAME_STMT); + gsi_insert_seq_before (gsi, new_stmt_list, GSI_SAME_STMT); } *initial_address = new_temp; @@ -5220,9 +5229,10 @@ vect_setup_realignment (gimple *stmt, gimple_stmt_iterator *gsi, NULL_TREE, loop); if (loop) { - pe = loop_preheader_edge (loop); + pe = loop_preheader_edge (loop); new_bb = gsi_insert_seq_on_edge_immediate (pe, stmts); gcc_assert (!new_bb); + bitmap_set_bit (changed_bbs, pe->src->index); } else gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT); diff --git a/gcc/tree-vect-loop-manip.c b/gcc/tree-vect-loop-manip.c index 0ff474d..4ad5ba8 100644 --- a/gcc/tree-vect-loop-manip.c +++ b/gcc/tree-vect-loop-manip.c @@ -1023,6 +1023,7 @@ vect_gen_prolog_loop_niters (loop_vec_info loop_vinfo, if (stmts) { gcc_assert (single_succ_p (bb)); + bitmap_set_bit (changed_bbs, bb->index); gimple_stmt_iterator gsi = gsi_last_bb (bb); if (gsi_end_p (gsi)) gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT); diff --git a/gcc/tree-vectorizer.c b/gcc/tree-vectorizer.c index f928dec..0f59dbc 100644 --- a/gcc/tree-vectorizer.c +++ b/gcc/tree-vectorizer.c @@ -74,6 +74,7 @@ along with GCC; see the file COPYING3. If not see #include "cfgloop.h" #include "tree-vectorizer.h" #include "tree-ssa-propagate.h" +#include "tree-ssa-dom.h" #include "dbgcnt.h" #include "tree-scalar-evolution.h" @@ -83,6 +84,9 @@ source_location vect_location; /* Vector mapping GIMPLE stmt to stmt_vec_info. */ vec<stmt_vec_info> stmt_vec_info_vec; + +/* Basic blocks should be cleaned up by CSE after vectorization. */ +bitmap changed_bbs; /* For mapping simduid to vectorization factor. */ @@ -540,6 +544,7 @@ vectorize_loops (void) note_simd_array_uses (&simd_array_to_simduid_htab); init_stmt_vec_info_vec (); + changed_bbs = BITMAP_ALLOC (NULL); /* ----------- Analyze loops. ----------- */ @@ -762,6 +767,9 @@ vectorize_loops (void) loop->aux = NULL; } + if (!bitmap_empty_p (changed_bbs)) + cse_bbs (changed_bbs); + BITMAP_FREE (changed_bbs); free_stmt_vec_info_vec (); /* Fold IFN_GOMP_SIMD_{VF,LANE,LAST_LANE,ORDERED_{START,END}} builtins. */ diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h index 12bb904..f16ab79 100644 --- a/gcc/tree-vectorizer.h +++ b/gcc/tree-vectorizer.h @@ -797,6 +797,7 @@ struct dataref_aux { && TYPE_UNSIGNED (TYPE))) extern vec<stmt_vec_info> stmt_vec_info_vec; +extern bitmap changed_bbs; void init_stmt_vec_info_vec (void); void free_stmt_vec_info_vec (void);