| Message ID | alpine.LSU.2.20.1908291202000.32458@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
| Series | Fix bootstrap miscompare by STV (PR91580) | expand |
On Thu, Aug 29, 2019 at 12:04 PM Richard Biener <rguenther@suse.de> wrote: > > > The following fixes the bootstrap-debug miscompare caused by STV > where we ended up with chain-to-scalar copies just because of > debug uses. Instead we have to avoid that, eventually substituting > into debug uses or resetting debug stmts when there are reaching > defs from both inside and outside of the chain (since we rename > all in-chain defs). > > Bootstrapped on i686-linux-gnu (with a setup previously > reproducing the miscompare). Bootstrapped on x86_64-unknown-linux-gnu, > testing in progress. > > OK for trunk? > > Thanks, > Richard. > > 2019-08-29 Richard Biener <rguenther@suse.de> > > PR bootstrap/91580 > * config/i386/i386-features.c (general_scalar_chain::convert_insn): > Do not emit scalar copies for debug-insns, instead replace > their uses with the reg copy used in the chain or reset them > if there is a reaching definition outside of the chain as well. OK. Let's fix the breakage, and maybe later we could look into merging with TImode debug fixups (which looks similar to the functionality in this patch). Thanks, Uros. > Index: gcc/config/i386/i386-features.c > =================================================================== > --- gcc/config/i386/i386-features.c (revision 274991) > +++ gcc/config/i386/i386-features.c (working copy) > @@ -880,18 +880,52 @@ general_scalar_chain::convert_op (rtx *o > void > general_scalar_chain::convert_insn (rtx_insn *insn) > { > - /* Generate copies for out-of-chain uses of defs. */ > + /* Generate copies for out-of-chain uses of defs and adjust debug uses. */ > for (df_ref ref = DF_INSN_DEFS (insn); ref; ref = DF_REF_NEXT_LOC (ref)) > if (bitmap_bit_p (defs_conv, DF_REF_REGNO (ref))) > { > df_link *use; > for (use = DF_REF_CHAIN (ref); use; use = use->next) > - if (DF_REF_REG_MEM_P (use->ref) > - || !bitmap_bit_p (insns, DF_REF_INSN_UID (use->ref))) > + if (NONDEBUG_INSN_P (DF_REF_INSN (use->ref)) > + && (DF_REF_REG_MEM_P (use->ref) > + || !bitmap_bit_p (insns, DF_REF_INSN_UID (use->ref)))) > break; > if (use) > convert_reg (insn, DF_REF_REG (ref), > *defs_map.get (regno_reg_rtx [DF_REF_REGNO (ref)])); > + else > + { > + /* If we generated a scalar copy we can leave debug-insns > + as-is, if not, we have to adjust them. */ > + auto_vec<rtx_insn *, 5> to_reset_debug_insns; > + for (use = DF_REF_CHAIN (ref); use; use = use->next) > + if (DEBUG_INSN_P (DF_REF_INSN (use->ref))) > + { > + rtx_insn *debug_insn = DF_REF_INSN (use->ref); > + /* If there's a reaching definition outside of the > + chain we have to reset. */ > + df_link *def; > + for (def = DF_REF_CHAIN (use->ref); def; def = def->next) > + if (!bitmap_bit_p (insns, DF_REF_INSN_UID (def->ref))) > + break; > + if (def) > + to_reset_debug_insns.safe_push (debug_insn); > + else > + { > + *DF_REF_REAL_LOC (use->ref) > + = *defs_map.get (regno_reg_rtx [DF_REF_REGNO (ref)]); > + df_insn_rescan (debug_insn); > + } > + } > + /* Have to do the reset outside of the DF_CHAIN walk to not > + disrupt it. */ > + while (!to_reset_debug_insns.is_empty ()) > + { > + rtx_insn *debug_insn = to_reset_debug_insns.pop (); > + INSN_VAR_LOCATION_LOC (debug_insn) = gen_rtx_UNKNOWN_VAR_LOC (); > + df_insn_rescan_debug_internal (debug_insn); > + } > + } > } > > /* Replace uses in this insn with the defs we use in the chain. */
On Thu, Aug 29, 2019 at 12:04:53PM +0200, Richard Biener wrote:
> + else
Perhaps use
else if (MAY_HAVE_DEBUG_BIND_INSNS)
instead so that you don't walk it once again if there can't be DEBUG_INSNs?
Jakub
On Thu, 29 Aug 2019, Jakub Jelinek wrote: > On Thu, Aug 29, 2019 at 12:04:53PM +0200, Richard Biener wrote: > > + else > > Perhaps use > else if (MAY_HAVE_DEBUG_BIND_INSNS) > instead so that you don't walk it once again if there can't be DEBUG_INSNs? Sure - will do as followup to unbreak bootstrap w/o re-testing this. Thanks, Richard.
On Thu, 29 Aug 2019, Uros Bizjak wrote: > On Thu, Aug 29, 2019 at 12:04 PM Richard Biener <rguenther@suse.de> wrote: > > > > > > The following fixes the bootstrap-debug miscompare caused by STV > > where we ended up with chain-to-scalar copies just because of > > debug uses. Instead we have to avoid that, eventually substituting > > into debug uses or resetting debug stmts when there are reaching > > defs from both inside and outside of the chain (since we rename > > all in-chain defs). > > > > Bootstrapped on i686-linux-gnu (with a setup previously > > reproducing the miscompare). Bootstrapped on x86_64-unknown-linux-gnu, > > testing in progress. > > > > OK for trunk? > > > > Thanks, > > Richard. > > > > 2019-08-29 Richard Biener <rguenther@suse.de> > > > > PR bootstrap/91580 > > * config/i386/i386-features.c (general_scalar_chain::convert_insn): > > Do not emit scalar copies for debug-insns, instead replace > > their uses with the reg copy used in the chain or reset them > > if there is a reaching definition outside of the chain as well. > > OK. r275030. > Let's fix the breakage, and maybe later we could look into merging > with TImode debug fixups (which looks similar to the functionality in > this patch). I don't think that's easily possible since the TImode chains still work in the way of having all defs of a pseudo in a chain, so code generation and replacement is different. Rather TImode chains could be handled by the generic machinery by only making loads, stores and reg-reg copies candidates. Richard. > Thanks, > Uros. > > > Index: gcc/config/i386/i386-features.c > > =================================================================== > > --- gcc/config/i386/i386-features.c (revision 274991) > > +++ gcc/config/i386/i386-features.c (working copy) > > @@ -880,18 +880,52 @@ general_scalar_chain::convert_op (rtx *o > > void > > general_scalar_chain::convert_insn (rtx_insn *insn) > > { > > - /* Generate copies for out-of-chain uses of defs. */ > > + /* Generate copies for out-of-chain uses of defs and adjust debug uses. */ > > for (df_ref ref = DF_INSN_DEFS (insn); ref; ref = DF_REF_NEXT_LOC (ref)) > > if (bitmap_bit_p (defs_conv, DF_REF_REGNO (ref))) > > { > > df_link *use; > > for (use = DF_REF_CHAIN (ref); use; use = use->next) > > - if (DF_REF_REG_MEM_P (use->ref) > > - || !bitmap_bit_p (insns, DF_REF_INSN_UID (use->ref))) > > + if (NONDEBUG_INSN_P (DF_REF_INSN (use->ref)) > > + && (DF_REF_REG_MEM_P (use->ref) > > + || !bitmap_bit_p (insns, DF_REF_INSN_UID (use->ref)))) > > break; > > if (use) > > convert_reg (insn, DF_REF_REG (ref), > > *defs_map.get (regno_reg_rtx [DF_REF_REGNO (ref)])); > > + else > > + { > > + /* If we generated a scalar copy we can leave debug-insns > > + as-is, if not, we have to adjust them. */ > > + auto_vec<rtx_insn *, 5> to_reset_debug_insns; > > + for (use = DF_REF_CHAIN (ref); use; use = use->next) > > + if (DEBUG_INSN_P (DF_REF_INSN (use->ref))) > > + { > > + rtx_insn *debug_insn = DF_REF_INSN (use->ref); > > + /* If there's a reaching definition outside of the > > + chain we have to reset. */ > > + df_link *def; > > + for (def = DF_REF_CHAIN (use->ref); def; def = def->next) > > + if (!bitmap_bit_p (insns, DF_REF_INSN_UID (def->ref))) > > + break; > > + if (def) > > + to_reset_debug_insns.safe_push (debug_insn); > > + else > > + { > > + *DF_REF_REAL_LOC (use->ref) > > + = *defs_map.get (regno_reg_rtx [DF_REF_REGNO (ref)]); > > + df_insn_rescan (debug_insn); > > + } > > + } > > + /* Have to do the reset outside of the DF_CHAIN walk to not > > + disrupt it. */ > > + while (!to_reset_debug_insns.is_empty ()) > > + { > > + rtx_insn *debug_insn = to_reset_debug_insns.pop (); > > + INSN_VAR_LOCATION_LOC (debug_insn) = gen_rtx_UNKNOWN_VAR_LOC (); > > + df_insn_rescan_debug_internal (debug_insn); > > + } > > + } > > } > > > > /* Replace uses in this insn with the defs we use in the chain. */ >
On Thu, 29 Aug 2019, Richard Biener wrote: > On Thu, 29 Aug 2019, Jakub Jelinek wrote: > > > On Thu, Aug 29, 2019 at 12:04:53PM +0200, Richard Biener wrote: > > > + else > > > > Perhaps use > > else if (MAY_HAVE_DEBUG_BIND_INSNS) > > instead so that you don't walk it once again if there can't be DEBUG_INSNs? > > Sure - will do as followup to unbreak bootstrap w/o re-testing this. Bootstrapped / tested on x86_64-unknown-linux-gnu, applied. Richard. 2019-08-29 Richard Biener <rguenther@suse.de> * config/i386/i386-features.c (general_scalar_chain::convert_insn): Guard debug work with MAY_HAVE_DEBUG_BIND_INSNS. Index: gcc/config/i386/i386-features.c =================================================================== --- gcc/config/i386/i386-features.c (revision 275030) +++ gcc/config/i386/i386-features.c (working copy) @@ -893,7 +893,7 @@ general_scalar_chain::convert_insn (rtx_ if (use) convert_reg (insn, DF_REF_REG (ref), *defs_map.get (regno_reg_rtx [DF_REF_REGNO (ref)])); - else + else if (MAY_HAVE_DEBUG_BIND_INSNS) { /* If we generated a scalar copy we can leave debug-insns as-is, if not, we have to adjust them. */
Index: gcc/config/i386/i386-features.c =================================================================== --- gcc/config/i386/i386-features.c (revision 274991) +++ gcc/config/i386/i386-features.c (working copy) @@ -880,18 +880,52 @@ general_scalar_chain::convert_op (rtx *o void general_scalar_chain::convert_insn (rtx_insn *insn) { - /* Generate copies for out-of-chain uses of defs. */ + /* Generate copies for out-of-chain uses of defs and adjust debug uses. */ for (df_ref ref = DF_INSN_DEFS (insn); ref; ref = DF_REF_NEXT_LOC (ref)) if (bitmap_bit_p (defs_conv, DF_REF_REGNO (ref))) { df_link *use; for (use = DF_REF_CHAIN (ref); use; use = use->next) - if (DF_REF_REG_MEM_P (use->ref) - || !bitmap_bit_p (insns, DF_REF_INSN_UID (use->ref))) + if (NONDEBUG_INSN_P (DF_REF_INSN (use->ref)) + && (DF_REF_REG_MEM_P (use->ref) + || !bitmap_bit_p (insns, DF_REF_INSN_UID (use->ref)))) break; if (use) convert_reg (insn, DF_REF_REG (ref), *defs_map.get (regno_reg_rtx [DF_REF_REGNO (ref)])); + else + { + /* If we generated a scalar copy we can leave debug-insns + as-is, if not, we have to adjust them. */ + auto_vec<rtx_insn *, 5> to_reset_debug_insns; + for (use = DF_REF_CHAIN (ref); use; use = use->next) + if (DEBUG_INSN_P (DF_REF_INSN (use->ref))) + { + rtx_insn *debug_insn = DF_REF_INSN (use->ref); + /* If there's a reaching definition outside of the + chain we have to reset. */ + df_link *def; + for (def = DF_REF_CHAIN (use->ref); def; def = def->next) + if (!bitmap_bit_p (insns, DF_REF_INSN_UID (def->ref))) + break; + if (def) + to_reset_debug_insns.safe_push (debug_insn); + else + { + *DF_REF_REAL_LOC (use->ref) + = *defs_map.get (regno_reg_rtx [DF_REF_REGNO (ref)]); + df_insn_rescan (debug_insn); + } + } + /* Have to do the reset outside of the DF_CHAIN walk to not + disrupt it. */ + while (!to_reset_debug_insns.is_empty ()) + { + rtx_insn *debug_insn = to_reset_debug_insns.pop (); + INSN_VAR_LOCATION_LOC (debug_insn) = gen_rtx_UNKNOWN_VAR_LOC (); + df_insn_rescan_debug_internal (debug_insn); + } + } } /* Replace uses in this insn with the defs we use in the chain. */