| Message ID | 87eh596cm4.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On 12/18/2013, 5:03 PM, Richard Sandiford wrote: > Hi Vlad, > > The initial LRA merge added the lra_in_progress check to this code > in simplify_subreg_regno: > > /* Give the backend a chance to disallow the mode change. */ > if (GET_MODE_CLASS (xmode) != MODE_COMPLEX_INT > && GET_MODE_CLASS (xmode) != MODE_COMPLEX_FLOAT > && REG_CANNOT_CHANGE_MODE_P (xregno, xmode, ymode) > /* We can use mode change in LRA for some transformations. */ > && ! lra_in_progress) > return -1; > > I realise that LRA internally uses subregs that wouldn't normally be valid > (e.g. to handle matching constraints) but I think it's really dangerous > to ignore REG_CANNOT_CHANGE_MODE_P and reduce a subreg to a specific > hard reg anyway. CLASS_CANNOT_CHANGE_MODE says that all bets are off in > terms of the normal subreg rules. E.g. the register class might have the > opposite endianness to GENERAL_REGS or something extreme like that. > As I remember the major reason for the change in simplify_subreg_regno was for dealing with some matching constraints representation in RTL (reload has no such problem as it does not need to represent it in RTL). Now we have LRA internal check for such subregs to differ it from original subregs. So I think we really could delete !lra_in_progress especially taking your arguments into account. > It wasn't obvious from the comment exactly what case this !lra_in_progress > was handling, but I tried reverting it and saw a failure in gcc.dg/pr49948.c. > The problem there was with a subreg of the form: > > (subreg:DI (reg:V2DI X) 0) > > We were trying to reload this subreg in GENERAL_REGS and X had been > allocated an SSE reg. We then hit this code in curr_insn_transform, > which was deciding whether to reload the reg or the subreg: > > if (REG_P (reg) > /* Strict_low_part requires reload the register not > the sub-register. */ > && (curr_static_id->operand[i].strict_low > || (GET_MODE_SIZE (mode) > <= GET_MODE_SIZE (GET_MODE (reg)) > && (hard_regno > = get_try_hard_regno (REGNO (reg))) >= 0 > && (simplify_subreg_regno > (hard_regno, > GET_MODE (reg), byte, mode) < 0) > && (goal_alt[i] == NO_REGS > || (simplify_subreg_regno > (ira_class_hard_regs[goal_alt[i]][0], > GET_MODE (reg), byte, mode) >= 0))))) > { > loc = &SUBREG_REG (*loc); > mode = GET_MODE (*loc); > } > > The first simplify_subreg_regno (on the SSE reg) used to return >= 0 > thanks to the !lra_in_progress, even though the change is usually supposed > to be disallowed. With the !lra_in_progress removed simplify_subreg_regno > rejects the simplification instead. Then the second simplify_subreg_regno > (on the first allocatable GENERAL_REGS) succeeds and we go on to reload > the inner reg. > > But the key in this case is that GENERAL_REGS isn't allowed to hold V2DImode, > so the simplification performed by the second simplify_subreg_regno > doesn't really have any meaning. I think we should check that > ira_class_hard_regs[goal_alt[i]][0] can hold GET_MODE (reg) before > trying to simplify a subreg of it. > It might work. By the way, I don't like situation in GCC that we have such check only on one hard reg base. For example, if constraint is AD_REGS regs, it is hard to understand why we can not hold V2DImode here. That is what could be improved in the future. > Tested on x86_64-linux-gnu ({,-m32}, all languages including Go and Ada). > Does it look OK? Or, if the check was originally added for a different > situation, are you still able to reproduce it? > > FWIW, this is all part of an attempt to fix the vec_select thing for x86_64. > I also want to get rid of the GET_MODE_CLASS checks seen in the first hunk, > but that's an entirely separate issue. > Thanks, Richard. For me, the patch is ok. But it is always hard to predict the result on all other platforms using LRA. Sometimes simple and obvious change for LRA/reload results in new problems -- I saw it several times. But I guess it will be not in this case. > gcc/ > * rtlanal.c (simplify_subreg_regno): Remove lra_in_progress check. > * lra-constraints.c (curr_insn_transform): Before trying to simplify > a subreg, check whether the original reg is valid. > > Index: gcc/rtlanal.c > =================================================================== > --- gcc/rtlanal.c 2013-12-09 08:03:31.931201686 +0000 > +++ gcc/rtlanal.c 2013-12-18 21:22:10.605987652 +0000 > @@ -3533,9 +3533,7 @@ simplify_subreg_regno (unsigned int xreg > /* Give the backend a chance to disallow the mode change. */ > if (GET_MODE_CLASS (xmode) != MODE_COMPLEX_INT > && GET_MODE_CLASS (xmode) != MODE_COMPLEX_FLOAT > - && REG_CANNOT_CHANGE_MODE_P (xregno, xmode, ymode) > - /* We can use mode change in LRA for some transformations. */ > - && ! lra_in_progress) > + && REG_CANNOT_CHANGE_MODE_P (xregno, xmode, ymode)) > return -1; > #endif > > Index: gcc/lra-constraints.c > =================================================================== > --- gcc/lra-constraints.c 2013-12-09 21:35:05.589588280 +0000 > +++ gcc/lra-constraints.c 2013-12-18 21:22:10.585987483 +0000 > @@ -3493,9 +3493,12 @@ curr_insn_transform (void) > (hard_regno, > GET_MODE (reg), byte, mode) < 0) > && (goal_alt[i] == NO_REGS > - || (simplify_subreg_regno > + || (HARD_REGNO_MODE_OK > (ira_class_hard_regs[goal_alt[i]][0], > - GET_MODE (reg), byte, mode) >= 0))))) > + GET_MODE (reg)) > + && (simplify_subreg_regno > + (ira_class_hard_regs[goal_alt[i]][0], > + GET_MODE (reg), byte, mode) >= 0)))))) > { > loc = &SUBREG_REG (*loc); > mode = GET_MODE (*loc); >
Index: gcc/rtlanal.c =================================================================== --- gcc/rtlanal.c 2013-12-09 08:03:31.931201686 +0000 +++ gcc/rtlanal.c 2013-12-18 21:22:10.605987652 +0000 @@ -3533,9 +3533,7 @@ simplify_subreg_regno (unsigned int xreg /* Give the backend a chance to disallow the mode change. */ if (GET_MODE_CLASS (xmode) != MODE_COMPLEX_INT && GET_MODE_CLASS (xmode) != MODE_COMPLEX_FLOAT - && REG_CANNOT_CHANGE_MODE_P (xregno, xmode, ymode) - /* We can use mode change in LRA for some transformations. */ - && ! lra_in_progress) + && REG_CANNOT_CHANGE_MODE_P (xregno, xmode, ymode)) return -1; #endif Index: gcc/lra-constraints.c =================================================================== --- gcc/lra-constraints.c 2013-12-09 21:35:05.589588280 +0000 +++ gcc/lra-constraints.c 2013-12-18 21:22:10.585987483 +0000 @@ -3493,9 +3493,12 @@ curr_insn_transform (void) (hard_regno, GET_MODE (reg), byte, mode) < 0) && (goal_alt[i] == NO_REGS - || (simplify_subreg_regno + || (HARD_REGNO_MODE_OK (ira_class_hard_regs[goal_alt[i]][0], - GET_MODE (reg), byte, mode) >= 0))))) + GET_MODE (reg)) + && (simplify_subreg_regno + (ira_class_hard_regs[goal_alt[i]][0], + GET_MODE (reg), byte, mode) >= 0)))))) { loc = &SUBREG_REG (*loc); mode = GET_MODE (*loc);
Hi Vlad, The initial LRA merge added the lra_in_progress check to this code in simplify_subreg_regno: /* Give the backend a chance to disallow the mode change. */ if (GET_MODE_CLASS (xmode) != MODE_COMPLEX_INT && GET_MODE_CLASS (xmode) != MODE_COMPLEX_FLOAT && REG_CANNOT_CHANGE_MODE_P (xregno, xmode, ymode) /* We can use mode change in LRA for some transformations. */ && ! lra_in_progress) return -1; I realise that LRA internally uses subregs that wouldn't normally be valid (e.g. to handle matching constraints) but I think it's really dangerous to ignore REG_CANNOT_CHANGE_MODE_P and reduce a subreg to a specific hard reg anyway. CLASS_CANNOT_CHANGE_MODE says that all bets are off in terms of the normal subreg rules. E.g. the register class might have the opposite endianness to GENERAL_REGS or something extreme like that. It wasn't obvious from the comment exactly what case this !lra_in_progress was handling, but I tried reverting it and saw a failure in gcc.dg/pr49948.c. The problem there was with a subreg of the form: (subreg:DI (reg:V2DI X) 0) We were trying to reload this subreg in GENERAL_REGS and X had been allocated an SSE reg. We then hit this code in curr_insn_transform, which was deciding whether to reload the reg or the subreg: if (REG_P (reg) /* Strict_low_part requires reload the register not the sub-register. */ && (curr_static_id->operand[i].strict_low || (GET_MODE_SIZE (mode) <= GET_MODE_SIZE (GET_MODE (reg)) && (hard_regno = get_try_hard_regno (REGNO (reg))) >= 0 && (simplify_subreg_regno (hard_regno, GET_MODE (reg), byte, mode) < 0) && (goal_alt[i] == NO_REGS || (simplify_subreg_regno (ira_class_hard_regs[goal_alt[i]][0], GET_MODE (reg), byte, mode) >= 0))))) { loc = &SUBREG_REG (*loc); mode = GET_MODE (*loc); } The first simplify_subreg_regno (on the SSE reg) used to return >= 0 thanks to the !lra_in_progress, even though the change is usually supposed to be disallowed. With the !lra_in_progress removed simplify_subreg_regno rejects the simplification instead. Then the second simplify_subreg_regno (on the first allocatable GENERAL_REGS) succeeds and we go on to reload the inner reg. But the key in this case is that GENERAL_REGS isn't allowed to hold V2DImode, so the simplification performed by the second simplify_subreg_regno doesn't really have any meaning. I think we should check that ira_class_hard_regs[goal_alt[i]][0] can hold GET_MODE (reg) before trying to simplify a subreg of it. Tested on x86_64-linux-gnu ({,-m32}, all languages including Go and Ada). Does it look OK? Or, if the check was originally added for a different situation, are you still able to reproduce it? FWIW, this is all part of an attempt to fix the vec_select thing for x86_64. I also want to get rid of the GET_MODE_CLASS checks seen in the first hunk, but that's an entirely separate issue. Thanks, Richard gcc/ * rtlanal.c (simplify_subreg_regno): Remove lra_in_progress check. * lra-constraints.c (curr_insn_transform): Before trying to simplify a subreg, check whether the original reg is valid.