On Sun, 29 May 2016, Tom de Vries wrote:
> Hi,
>
> this patch fixes graphite PR69067, a 6/7 regression.
>
>
> I.
>
> Consider the following test-case, compiled with -O1 -floop-nest-optimize
> -flto:
> ...
> int a1, c1, cr, kt;
> int aa[2];
>
> int
> ce (void)
> {
> while (a1 < 1) {
> int g8;
> for (g8 = 0; g8 < 3; ++g8)
> if (c1 != 0)
> cr = aa[a1 * 2] = kt;
> for (c1 = 0; c1 < 2; ++c1)
> aa[c1] = cr;
> ++a1;
> }
> return 0;
> }
>
> int
> main (void)
> {
> return ce (aa);
> }
> ...
>
> At graphite0, there's a loop with header bb4, which conditionally executes bb
> 5:
> ...
> <bb 3>:
>
> <bb 4>:
> # g8_39 = PHI <0(13), g8_19(3)>
> # cr_lsm.1_4 = PHI <cr_lsm.1_21(13), cr_lsm.1_33(3)>
> # cr_lsm.2_22 = PHI <cr_lsm.2_23(13), cr_lsm.2_11(3)>
> if (c1_lsm.3_35 != 0)
> goto <bb 5>;
> else
> goto <bb 6>;
>
> <bb 5>:
> aa[_3] = 0;
>
> <bb 6>:
> # cr_lsm.1_33 = PHI <cr_lsm.1_4(4), 0(5)>
> # cr_lsm.2_11 = PHI <cr_lsm.2_22(4), 1(5)>
> g8_19 = g8_39 + 1;
> if (g8_19 <= 2)
> goto <bb 3>;
> else
> goto <bb 7>;
> ...
>
>
> II.
>
> The graphite transformation moves the condition '(P_35 <= -1 || P_35 >= 1)'
> out of the loop:
> ...
> [scheduler] original ast:
> {
> for (int c0 = 0; c0 <= 2; c0 += 1) {
> S_4(c0);
> if (P_35 <= -1 || P_35 >= 1)
> S_5(c0);
> S_6(c0);
> }
> S_7();
> for (int c0 = 0; c0 <= 1; c0 += 1)
> S_8(c0);
> }
>
> [scheduler] AST generated by isl:
> {
> if (P_35 <= -1) {
> for (int c0 = 0; c0 <= 2; c0 += 1)
> S_5(c0);
> } else if (P_35 >= 1)
> for (int c0 = 0; c0 <= 2; c0 += 1)
> S_5(c0);
> for (int c0 = 0; c0 <= 2; c0 += 1) {
> S_4(c0);
> S_6(c0);
> }
> S_7();
> for (int c0 = 0; c0 <= 1; c0 += 1)
> S_8(c0);
> }
> ...
>
> When instantiating the ast back to gimple, we run into an assert:
> ...
> pr-graphite-4.c: In function ‘ce’:
> pr-graphite-4.c:5:1: internal compiler error: in get_def_bb_for_const, at
> graphite-isl-ast-to-gimple.c:1795
> ce()
> ^
> ...
>
>
> III.
>
> What happens is the following: in copy_cond_phi_args we try to copy the
> arguments of phi in bb6 to the arguments of new_phi in bb 46
> ...
> (gdb) call debug_gimple_stmt (phi)
> cr_lsm.1_33 = PHI <cr_lsm.1_4(4), 0(5)>
> (gdb) call debug_gimple_stmt (new_phi)
> cr_lsm.1_62 = PHI <(28), (47)>
> ...
>
> While handling the '0' phi argument in add_phi_arg_for_new_expr we trigger
> this bit of code and call get_def_bb_for_const with bb.index == 46 and
> old_bb.index == 5:
> ...
> /* If the corresponding def_bb could not be found the entry will
> be NULL. */
> if (TREE_CODE (old_phi_args[i]) == INTEGER_CST)
> def_pred[i]
> = get_def_bb_for_const (new_bb,
> gimple_phi_arg_edge (phi, i)->src);
> ...
>
> Neither of the two copies of bb 5 dominates bb 46, so we run into the assert
> at the end:
> ...
> /* Returns a basic block that could correspond to where a constant was
> defined in the original code. In the original code OLD_BB had the
> definition, we need to find which basic block out of the copies of
> old_bb, in the new region, should a definition correspond to if it
> has to reach BB. */
>
> basic_block translate_isl_ast_to_gimple::
> get_def_bb_for_const (basic_block bb, basic_block old_bb) const
> {
> vec <basic_block> *bbs = region->copied_bb_map->get (old_bb);
>
> if (!bbs || bbs->is_empty ())
> return NULL;
>
> if (1 == bbs->length ())
> return (*bbs)[0];
>
> int i;
> basic_block b1 = NULL, b2;
> FOR_EACH_VEC_ELT (*bbs, i, b2)
> {
> if (b2 == bb)
> return bb;
>
> /* BB and B2 are in two unrelated if-clauses. */
> if (!dominated_by_p (CDI_DOMINATORS, bb, b2))
> continue;
>
> /* Compute the nearest dominator. */
> if (!b1 || dominated_by_p (CDI_DOMINATORS, b2, b1))
> b1 = b2;
> }
>
> gcc_assert (b1);
> return b1;
> }
> ...
>
>
> IV.
>
> Attached patch fixes this by removing the assert.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk, 6-branch?
Ok.
Thanks,
Richard.
> Thanks,
> - Tom
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
