| Message ID | E0DA9601-F342-4506-B2F3-502DDAA9BD34@comcast.net |
|---|---|
| State | New |
| Headers | show |
On Fri, Jan 30, 2015 at 09:45:26AM -0800, Mike Stump wrote: > I have a port that has: > > (insn 47 46 48 18 (parallel [ > (unspec_volatile:DI [ > (const_int 128 [0x80]) > (const_int 6 [0x6]) > ] UNSPECV_SPECIAL_OP) > (set (mem/v:BLK (scratch:DI) [0 A8]) > (unspec:BLK [ > (mem/v:BLK (scratch:DI) [0 A8]) > ] UNSPEC_MEMORY_BARRIER)) Why don't you just (clobber (mem/v:BLK (scratch:DI))) instead of the second set in the parallel? The instruction is already UNSPEC_VOLATILE, and to make the barrier effect clear a clobber should be sufficient. Jakub
On Jan 30, 2015, at 9:52 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Jan 30, 2015 at 09:45:26AM -0800, Mike Stump wrote: >> I have a port that has: >> >> (insn 47 46 48 18 (parallel [ >> (unspec_volatile:DI [ >> (const_int 128 [0x80]) >> (const_int 6 [0x6]) >> ] UNSPECV_SPECIAL_OP) >> (set (mem/v:BLK (scratch:DI) [0 A8]) >> (unspec:BLK [ >> (mem/v:BLK (scratch:DI) [0 A8]) >> ] UNSPEC_MEMORY_BARRIER)) > > Why don't you just (clobber (mem/v:BLK (scratch:DI))) instead > of the second set in the parallel? The instruction is already > UNSPEC_VOLATILE, and to make the barrier effect clear a clobber should be > sufficient. So, f087d65d84655bc2d5fdd4bcc6bf0fe337a39893 seems to have introduced the current way that all the ports do this currently. So, I’d leave this to Richard to answer. If (clobber (mem/v:BLK (scratch:DI))) works, certainly that seems simpler than what people do now, but, then I’m left wondering, why didn’t we do that from that start?
On 01/30/2015 12:12 PM, Mike Stump wrote: > On Jan 30, 2015, at 9:52 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Fri, Jan 30, 2015 at 09:45:26AM -0800, Mike Stump wrote: >>> I have a port that has: >>> >>> (insn 47 46 48 18 (parallel [ >>> (unspec_volatile:DI [ >>> (const_int 128 [0x80]) >>> (const_int 6 [0x6]) >>> ] UNSPECV_SPECIAL_OP) >>> (set (mem/v:BLK (scratch:DI) [0 A8]) >>> (unspec:BLK [ >>> (mem/v:BLK (scratch:DI) [0 A8]) >>> ] UNSPEC_MEMORY_BARRIER)) >> >> Why don't you just (clobber (mem/v:BLK (scratch:DI))) instead >> of the second set in the parallel? The instruction is already >> UNSPEC_VOLATILE, and to make the barrier effect clear a clobber should be >> sufficient. > > So, f087d65d84655bc2d5fdd4bcc6bf0fe337a39893 seems to have introduced the current way that all the ports do this currently. So, I’d leave this to Richard to answer. > > If (clobber (mem/v:BLK (scratch:DI))) works, certainly that seems simpler than what people do now, but, then I’m left wondering, why didn’t we do that from that start? > Jakub, the current formation includes both a use and a set of all memory. Your clobber form would not imply a use. r~
On January 30, 2015 9:12:12 PM CET, Mike Stump <mikestump@comcast.net> wrote: >On Jan 30, 2015, at 9:52 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Fri, Jan 30, 2015 at 09:45:26AM -0800, Mike Stump wrote: >>> I have a port that has: >>> >>> (insn 47 46 48 18 (parallel [ >>> (unspec_volatile:DI [ >>> (const_int 128 [0x80]) >>> (const_int 6 [0x6]) >>> ] UNSPECV_SPECIAL_OP) >>> (set (mem/v:BLK (scratch:DI) [0 A8]) >>> (unspec:BLK [ >>> (mem/v:BLK (scratch:DI) [0 A8]) >>> ] UNSPEC_MEMORY_BARRIER)) >> >> Why don't you just (clobber (mem/v:BLK (scratch:DI))) instead >> of the second set in the parallel? The instruction is already >> UNSPEC_VOLATILE, and to make the barrier effect clear a clobber >should be >> sufficient. > >So, f087d65d84655bc2d5fdd4bcc6bf0fe337a39893 seems to have introduced >the current way that all the ports do this currently. So, I’d leave >this to Richard to answer. > >If (clobber (mem/v:BLK (scratch:DI))) works, certainly that seems >simpler than what people do now, but, then I’m left wondering, why >didn’t we do that from that start? There would be the subtle difference that the set keeps all previous memory live while the clobber is only a barrier for memory instruction movement. Richard.
On January 30, 2015 9:18:57 PM CET, Richard Henderson <rth@redhat.com> wrote: >On 01/30/2015 12:12 PM, Mike Stump wrote: >> On Jan 30, 2015, at 9:52 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>> On Fri, Jan 30, 2015 at 09:45:26AM -0800, Mike Stump wrote: >>>> I have a port that has: >>>> >>>> (insn 47 46 48 18 (parallel [ >>>> (unspec_volatile:DI [ >>>> (const_int 128 [0x80]) >>>> (const_int 6 [0x6]) >>>> ] UNSPECV_SPECIAL_OP) >>>> (set (mem/v:BLK (scratch:DI) [0 A8]) >>>> (unspec:BLK [ >>>> (mem/v:BLK (scratch:DI) [0 A8]) >>>> ] UNSPEC_MEMORY_BARRIER)) >>> >>> Why don't you just (clobber (mem/v:BLK (scratch:DI))) instead >>> of the second set in the parallel? The instruction is already >>> UNSPEC_VOLATILE, and to make the barrier effect clear a clobber >should be >>> sufficient. >> >> So, f087d65d84655bc2d5fdd4bcc6bf0fe337a39893 seems to have introduced >the current way that all the ports do this currently. So, I’d leave >this to Richard to answer. >> >> If (clobber (mem/v:BLK (scratch:DI))) works, certainly that seems >simpler than what people do now, but, then I’m left wondering, why >didn’t we do that from that start? >> > >Jakub, the current formation includes both a use and a set of all >memory. Your >clobber form would not imply a use. What do you need the use for? Prevent all DSE before the barrier for some weird reason? Richard. > >r~
On 01/30/2015 12:40 PM, Richard Biener wrote: > On January 30, 2015 9:18:57 PM CET, Richard Henderson <rth@redhat.com> wrote: >> On 01/30/2015 12:12 PM, Mike Stump wrote: >>> On Jan 30, 2015, at 9:52 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>> On Fri, Jan 30, 2015 at 09:45:26AM -0800, Mike Stump wrote: >>>>> I have a port that has: >>>>> >>>>> (insn 47 46 48 18 (parallel [ >>>>> (unspec_volatile:DI [ >>>>> (const_int 128 [0x80]) >>>>> (const_int 6 [0x6]) >>>>> ] UNSPECV_SPECIAL_OP) >>>>> (set (mem/v:BLK (scratch:DI) [0 A8]) >>>>> (unspec:BLK [ >>>>> (mem/v:BLK (scratch:DI) [0 A8]) >>>>> ] UNSPEC_MEMORY_BARRIER)) >>>> >>>> Why don't you just (clobber (mem/v:BLK (scratch:DI))) instead >>>> of the second set in the parallel? The instruction is already >>>> UNSPEC_VOLATILE, and to make the barrier effect clear a clobber >> should be >>>> sufficient. >>> >>> So, f087d65d84655bc2d5fdd4bcc6bf0fe337a39893 seems to have introduced >> the current way that all the ports do this currently. So, I’d leave >> this to Richard to answer. >>> >>> If (clobber (mem/v:BLK (scratch:DI))) works, certainly that seems >> simpler than what people do now, but, then I’m left wondering, why >> didn’t we do that from that start? >>> >> >> Jakub, the current formation includes both a use and a set of all >> memory. Your >> clobber form would not imply a use. > > What do you need the use for? Prevent all DSE before the barrier for some weird reason? I don't consider the clobber to be accurately representational of a barrier. It may work by accident, because the scratch address prevents aliasing disambiguation, but I don't think it's good form. If we were talking about a register and not memory, the clobber would not prevent movement of a store across the barrier. Do we really want different rules for (mem (scratch)) than other rtl objects? r~
On Fri, Jan 30, 2015 at 12:50:01PM -0800, Richard Henderson wrote: > >> Jakub, the current formation includes both a use and a set of all > >> memory. Your > >> clobber form would not imply a use. > > > > What do you need the use for? Prevent all DSE before the barrier for some weird reason? > > I don't consider the clobber to be accurately representational of a barrier. > It may work by accident, because the scratch address prevents aliasing > disambiguation, but I don't think it's good form. alias.c says: /* (mem:BLK (scratch)) is a special mechanism to conflict with everything. This is used in epilogue deallocation functions, and in cselib. */ so it is not an accident? You added it yourself, back in 2002 :-) The commit message mentions PR6165, which leads us to http://gcc.gnu.org/ml/gcc-patches/2002-04/msg00231.html > If we were talking about a register and not memory, the clobber would not > prevent movement of a store across the barrier. Because registers do not live in memory (as far as GCC knows, anyway)? You can also clobber some specific mem, and that does prevent movement of any store to (or read from) that mem over the clobber. > Do we really want different > rules for (mem (scratch)) than other rtl objects? It is quite similar to related things, in most ways (even the "scratch" part reads similar to "(clobber (scratch))"). There is nothing else that will give these semantics. We could invent a new RTL code, with the only effect that we would need to check for more codes in more places -- there aren't many places that need to check for this merely to exclude it. Segher
On January 31, 2015 4:07:23 AM CET, Segher Boessenkool <segher@kernel.crashing.org> wrote: >On Fri, Jan 30, 2015 at 12:50:01PM -0800, Richard Henderson wrote: >> >> Jakub, the current formation includes both a use and a set of all >> >> memory. Your >> >> clobber form would not imply a use. >> > >> > What do you need the use for? Prevent all DSE before the barrier >for some weird reason? >> >> I don't consider the clobber to be accurately representational of a >barrier. >> It may work by accident, because the scratch address prevents >aliasing >> disambiguation, but I don't think it's good form. > >alias.c says: > >/* (mem:BLK (scratch)) is a special mechanism to conflict with >everything. > This is used in epilogue deallocation functions, and in cselib. */ > >so it is not an accident? You added it yourself, back in 2002 :-) >The commit message mentions PR6165, which leads us to >http://gcc.gnu.org/ml/gcc-patches/2002-04/msg00231.html > >> If we were talking about a register and not memory, the clobber would >not >> prevent movement of a store across the barrier. > >Because registers do not live in memory (as far as GCC knows, anyway)? >You >can also clobber some specific mem, and that does prevent movement of >any >store to (or read from) that mem over the clobber. > >> Do we really want different >> rules for (mem (scratch)) than other rtl objects? > >It is quite similar to related things, in most ways (even the "scratch" >part >reads similar to "(clobber (scratch))"). > >There is nothing else that will give these semantics. We could invent >a >new RTL code, with the only effect that we would need to check for more >codes in more places -- there aren't many places that need to check for >this merely to exclude it. The only relevant thing is that aliasing cannot disambiguate the MEM that is clobbered. Whether that's via using a scratch or some other means is not relevant. Having a MEM use pessimizes code in addition to being a memory barrier. Where memory barrier == scheduling barrier. Richard. > >Segher
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c index 382281c..c82aa3f 100644 --- a/gcc/lra-constraints.c +++ b/gcc/lra-constraints.c @@ -2855,6 +2855,7 @@ process_address_1 (int nop, bool check_only_p, if (! check_only_p) change_p = equiv_address_substitution (&ad); if (ad.base_term != NULL + && GET_CODE (*ad.base_term) != SCRATCH && (process_addr_reg (ad.base_term, check_only_p, before, (ad.autoinc_p @@ -2898,7 +2899,9 @@ process_address_1 (int nop, bool check_only_p, All these cases involve a non-autoinc address, so there is no point revalidating other types. */ - if (ad.autoinc_p || valid_address_p (&ad)) + if (ad.autoinc_p + || (ad.base_term && GET_CODE (*ad.base_term) == SCRATCH) + || valid_address_p (&ad)) return change_p; /* Any index existed before LRA started, so we can assume that the
I have a port that has: (insn 47 46 48 18 (parallel [ (unspec_volatile:DI [ (const_int 128 [0x80]) (const_int 6 [0x6]) ] UNSPECV_SPECIAL_OP) (set (mem/v:BLK (scratch:DI) [0 A8]) (unspec:BLK [ (mem/v:BLK (scratch:DI) [0 A8]) ] UNSPEC_MEMORY_BARRIER)) The UNSPEC_MEMORY_BARRIER is the usual memory barrier, and the other unspec is a random complex instruction that the optimizer will never have a clue about. I caught lra trying to reload the scratch address into a register and then aborting later as it isn’t a use, a clobber or an asm_input. The code that asserts this is: /* It is a special insn like USE or CLOBBER. We should recognize any regular insn otherwise LRA can do nothing with this insn. */ gcc_assert (GET_CODE (PATTERN (insn)) == USE || GET_CODE (PATTERN (insn)) == CLOBBER || GET_CODE (PATTERN (insn)) == ASM_INPUT); in lra.c I notice that if I put in the below, I can get lra to not do the reload. I get the feeling that I’m missing something from my port someplace else that would have prevented lra from doing this, as we have a ton of ports that have this form of memory barrier. avr nopv is one such instance. [ checking ] Oh, avr is one such port, but, it doesn’t use lra. :-( Hum… So, I guess the question is, is this an lra bug, or a port bug? When I use the avr patterns for nopv on an lra port, the pattern then doesn’t work.