| Message ID | f59df31d485d71c8e4c68c74ef1ca1dc4a9dd8d4.1542381960.git.ams@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | AMD GCN Port v2 | expand |
On 11/16/18 9:27 AM, Andrew Stubbs wrote: > > This patch is unchanged from that which was posted before. Discussion > fizzled out there and I was too busy with other patches to restart it > then. This issue needs to be resolved before libgfortran can be > compiled for GCN. > > The IRA pass makes an assumption that any pseudos created after the pass begins > were created explicitly by the pass itself and therefore will have > corresponding entries in its other tables. > > The GCN back-end, however, often creates additional pseudos, in expand > patterns, to represent the necessary EXEC value, and these break IRA's > assumption and cause ICEs: > > ..../libgfortran/generated/matmul_r8.c: In function 'matmul_r8': > ..../libgfortran/generated/matmul_r8.c:3002:1: internal compiler error: in setup_preferred_alternate_classes_for_new_pseudos, at ira.c:2772 > > This patch simply has IRA skip unknown pseudos, and the problem goes away. > > Presumably, it's not ideal that these registers have not been processed by IRA, > but it does not appear to do any real harm. > > 2018-11-16 Andrew Stubbs <ams@codesourcery.com> > > gcc/ > * ira.c (setup_preferred_alternate_classes_for_new_pseudos): Skip > pseudos not created by this pass. > (move_unallocated_pseudos): Likewise. This seems like a really gross hack and sets an expectation that generating registers in the target after IRA has started is OK. It is not OK. THe fact that this works is, IMHO, likely an accident. I think this comes back to the fundamental representational issue with the EXEC handling that still needs to be addressed. Jeff
On 21/11/2018 00:47, Jeff Law wrote: > This seems like a really gross hack and sets an expectation that > generating registers in the target after IRA has started is OK. It is > not OK. THe fact that this works is, IMHO, likely an accident. What's the proper test for this? Neither lra_in_progress nor reload_in_progress is set here, and can_create_pseudos returns true. The patterns have the ability to not generate registers, but they don't know not to. Richard Sandiford has stated that it should be OK, but perhaps the other architectures also work by accident? In fact, since we're using LRA (not reload), my understanding is that I ought to be able to create new pseudos right up until reload_completed. (Although, my experience is that it's easy to get into an infinite loop doing that.) > I think this comes back to the fundamental representational issue with > the EXEC handling that still needs to be addressed. Undoubtedly, this makes it worse, but even without that I'd still want to expand vector memory moves long before split1, so at least some cases have to generate additional registers. (Perhaps IRA doesn't create memory moves though? I'm not sure.) I'm going to investigate how easy it is to fix the EXEC representation issues. I've been resisting because I had a deadline to make, and it's bound to be an invasive and destabilizing alteration (albeit largely mechanical), but if it's going to be a barrier to commit then probably it's become time. :-( Andrew
Andrew Stubbs <ams@codesourcery.com> writes: > On 21/11/2018 00:47, Jeff Law wrote: >> This seems like a really gross hack and sets an expectation that >> generating registers in the target after IRA has started is OK. It is >> not OK. THe fact that this works is, IMHO, likely an accident. > > What's the proper test for this? Neither lra_in_progress nor > reload_in_progress is set here, and can_create_pseudos returns true. > > The patterns have the ability to not generate registers, but they don't > know not to. > > Richard Sandiford has stated that it should be OK, but perhaps the other > architectures also work by accident? Yeah, my understanding was that targets could create new registers here, and I thought targets did in some situations. But that's also why I'm suspicious of the patch. If GCN is doing something valid and IRA isn't coping, then the patch seems to be fixing the problem in the wrong place. So I still agree with Jeff on the gross hack thing... Richard > In fact, since we're using LRA (not reload), my understanding is that I > ought to be able to create new pseudos right up until reload_completed. > (Although, my experience is that it's easy to get into an infinite loop > doing that.) > >> I think this comes back to the fundamental representational issue with >> the EXEC handling that still needs to be addressed. > > Undoubtedly, this makes it worse, but even without that I'd still want > to expand vector memory moves long before split1, so at least some cases > have to generate additional registers. (Perhaps IRA doesn't create > memory moves though? I'm not sure.) > > I'm going to investigate how easy it is to fix the EXEC representation > issues. I've been resisting because I had a deadline to make, and it's > bound to be an invasive and destabilizing alteration (albeit largely > mechanical), but if it's going to be a barrier to commit then probably > it's become time. :-( > > Andrew
On 12/8/18 5:14 AM, Richard Sandiford wrote: > Andrew Stubbs <ams@codesourcery.com> writes: >> On 21/11/2018 00:47, Jeff Law wrote: >>> This seems like a really gross hack and sets an expectation that >>> generating registers in the target after IRA has started is OK. It is >>> not OK. THe fact that this works is, IMHO, likely an accident. >> >> What's the proper test for this? Neither lra_in_progress nor >> reload_in_progress is set here, and can_create_pseudos returns true. >> >> The patterns have the ability to not generate registers, but they don't >> know not to. >> >> Richard Sandiford has stated that it should be OK, but perhaps the other >> architectures also work by accident? > > Yeah, my understanding was that targets could create new registers here, > and I thought targets did in some situations. But that's also why I'm > suspicious of the patch. If GCN is doing something valid and IRA isn't > coping, then the patch seems to be fixing the problem in the wrong place. IRA/LRA can create new pseudos internally. What I'm much less sure about is whether or not the target can create them. WHen IRA/LRA creates one internally it has the chance to update the various internal structures it needs. That can't happen with a pseudo created by the target this late. Vlad would know for sure. jeff
On 08/12/2018 16:23, Jeff Law wrote: > On 12/8/18 5:14 AM, Richard Sandiford wrote: >> Andrew Stubbs <ams@codesourcery.com> writes: >>> On 21/11/2018 00:47, Jeff Law wrote: >>>> This seems like a really gross hack and sets an expectation that >>>> generating registers in the target after IRA has started is OK. It is >>>> not OK. THe fact that this works is, IMHO, likely an accident. >>> >>> What's the proper test for this? Neither lra_in_progress nor >>> reload_in_progress is set here, and can_create_pseudos returns true. >>> >>> The patterns have the ability to not generate registers, but they don't >>> know not to. >>> >>> Richard Sandiford has stated that it should be OK, but perhaps the other >>> architectures also work by accident? >> >> Yeah, my understanding was that targets could create new registers here, >> and I thought targets did in some situations. But that's also why I'm >> suspicious of the patch. If GCN is doing something valid and IRA isn't >> coping, then the patch seems to be fixing the problem in the wrong place. > IRA/LRA can create new pseudos internally. What I'm much less sure > about is whether or not the target can create them. WHen IRA/LRA > creates one internally it has the chance to update the various internal > structures it needs. That can't happen with a pseudo created by the > target this late. Vlad would know for sure. In any case, the new patch series that I will post soon does not appear to need this patch any more. (I'll post that as soon as I figure out an unrelated problem with jump threading.) Andrew
diff --git a/gcc/ira.c b/gcc/ira.c index def194a..e0c293c 100644 --- a/gcc/ira.c +++ b/gcc/ira.c @@ -2769,7 +2769,12 @@ setup_preferred_alternate_classes_for_new_pseudos (int start) for (i = start; i < max_regno; i++) { old_regno = ORIGINAL_REGNO (regno_reg_rtx[i]); - ira_assert (i != old_regno); + + /* Skip any new pseudos not created directly by this pass. + gen_move_insn can do this on AMD GCN, for example. */ + if (i == old_regno) + continue; + setup_reg_classes (i, reg_preferred_class (old_regno), reg_alternate_class (old_regno), reg_allocno_class (old_regno)); @@ -5054,6 +5059,12 @@ move_unallocated_pseudos (void) { int idx = i - first_moveable_pseudo; rtx other_reg = pseudo_replaced_reg[idx]; + + /* Skip any new pseudos not created directly by find_moveable_pseudos. + gen_move_insn can do this on AMD GCN, for example. */ + if (!other_reg) + continue; + rtx_insn *def_insn = DF_REF_INSN (DF_REG_DEF_CHAIN (i)); /* The use must follow all definitions of OTHER_REG, so we can insert the new definition immediately after any of them. */