| Message ID | alpine.LSU.2.20.1611231423290.31313@wotan.suse.de |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 23, 2016 at 02:26:49PM +0100, Michael Matz wrote: > Hi, > > as the bug trail explains make_extraction is claiming but failing to > handle extractions that would go outside the underlying object. So, let's > not construct such, as the patch does. > > Dominik tested s390x bootstrap being recovered with this, Andreas ia64 > bootstrap, and I regstrapped this on x86-64-linux without regressions (all > langs+ada). Okay for trunk? Shouldn't new_rtx be set to NULL_RTX if that condition is false? Otherwise it will be whatever make_compound_operation returned. What about the break? Shouldn't that be done only if the condition is true too? Anyway, I'm afraid I don't know this code enough, so deferring to Segher. > PR bootstrap/78390 > * combine.c (make_compound_operation_int): Don't extract > from outside underlying object. > > diff --git a/gcc/combine.c b/gcc/combine.c > index 0210685..1d8bddf 100644 > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -8108,9 +8108,16 @@ make_compound_operation (rtx x, enum rtx_code in_code) > && GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (inner)) > && subreg_lowpart_p (x)) > { > + int len = mode_width; > new_rtx = make_compound_operation (XEXP (inner, 0), next_code); > + /* Don't extract bits outside the underlying mode. */ > + if (CONST_INT_P (XEXP (inner, 1)) > + && (INTVAL (XEXP (inner, 1)) + len > + > GET_MODE_PRECISION (GET_MODE (new_rtx)))) > + len = GET_MODE_PRECISION (GET_MODE (new_rtx)) > + - INTVAL (XEXP (inner, 1)); > new_rtx = make_extraction (mode, new_rtx, 0, XEXP (inner, 1), > - mode_width, 1, 0, in_code == COMPARE); > + len, 1, 0, in_code == COMPARE); > break; > } > Jakub
Hi, On Wed, 23 Nov 2016, Jakub Jelinek wrote: > On Wed, Nov 23, 2016 at 02:26:49PM +0100, Michael Matz wrote: > > Hi, > > > > as the bug trail explains make_extraction is claiming but failing to > > handle extractions that would go outside the underlying object. So, let's > > not construct such, as the patch does. > > > > Dominik tested s390x bootstrap being recovered with this, Andreas ia64 > > bootstrap, and I regstrapped this on x86-64-linux without regressions (all > > langs+ada). Okay for trunk? > > Shouldn't new_rtx be set to NULL_RTX if that condition is false? Otherwise > it will be whatever make_compound_operation returned. What about the break? > Shouldn't that be done only if the condition is true too? Hmm? The conditionial statement only guards the adjustment of len if that happens to be too large to be completely contained inside new_rtx. If it's not outside len doesn't need adjustment. Yes, new_rtx will always be whatever make_compound_operation returns (no matter the condition) and be the input to make_extraction as the object from which to extract (which itself will then either return NULL_RTX if the asked extraction is unfeasible or the extracted object). The break is there to not enter the following code on a (subreg (lshiftrt )). It could be conditionalized on new_rtx being != NULL (so that it only breaks if the extraction was successfull and otherwise falls through to the generic subreg code). But that would be a different change not having to do with the bootstrap problem. (It would make sense from my POV). Ciao, Michael. > > Anyway, I'm afraid I don't know this code enough, so deferring to Segher. > > > PR bootstrap/78390 > > * combine.c (make_compound_operation_int): Don't extract > > from outside underlying object. > > > > diff --git a/gcc/combine.c b/gcc/combine.c > > index 0210685..1d8bddf 100644 > > --- a/gcc/combine.c > > +++ b/gcc/combine.c > > @@ -8108,9 +8108,16 @@ make_compound_operation (rtx x, enum rtx_code in_code) > > && GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (inner)) > > && subreg_lowpart_p (x)) > > { > > + int len = mode_width; > > new_rtx = make_compound_operation (XEXP (inner, 0), next_code); > > + /* Don't extract bits outside the underlying mode. */ > > + if (CONST_INT_P (XEXP (inner, 1)) > > + && (INTVAL (XEXP (inner, 1)) + len > > + > GET_MODE_PRECISION (GET_MODE (new_rtx)))) > > + len = GET_MODE_PRECISION (GET_MODE (new_rtx)) > > + - INTVAL (XEXP (inner, 1)); > > new_rtx = make_extraction (mode, new_rtx, 0, XEXP (inner, 1), > > - mode_width, 1, 0, in_code == COMPARE); > > + len, 1, 0, in_code == COMPARE); > > break; > > } > > > > Jakub >
On Wed, Nov 23, 2016 at 02:45:23PM +0100, Michael Matz wrote: > > Shouldn't new_rtx be set to NULL_RTX if that condition is false? Otherwise > > it will be whatever make_compound_operation returned. What about the break? > > Shouldn't that be done only if the condition is true too? > > Hmm? The conditionial statement only guards the adjustment of len if that > happens to be too large to be completely contained inside new_rtx. new_rtx shouldn't be set at all here for non-constant shifts, because those have the same problem as this new conditional tries to solve. I'm committing a similar patch, see other thread. Segher
diff --git a/gcc/combine.c b/gcc/combine.c index 0210685..1d8bddf 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -8108,9 +8108,16 @@ make_compound_operation (rtx x, enum rtx_code in_code) && GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (inner)) && subreg_lowpart_p (x)) { + int len = mode_width; new_rtx = make_compound_operation (XEXP (inner, 0), next_code); + /* Don't extract bits outside the underlying mode. */ + if (CONST_INT_P (XEXP (inner, 1)) + && (INTVAL (XEXP (inner, 1)) + len + > GET_MODE_PRECISION (GET_MODE (new_rtx)))) + len = GET_MODE_PRECISION (GET_MODE (new_rtx)) + - INTVAL (XEXP (inner, 1)); new_rtx = make_extraction (mode, new_rtx, 0, XEXP (inner, 1), - mode_width, 1, 0, in_code == COMPARE); + len, 1, 0, in_code == COMPARE); break; }