Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, Segher Boessenkool writes: > On Thu, Jun 15, 2023 at 03:00:40PM +0800, Jiufu Guo wrote: >> >> This is the existing pattern. It may be read as an action >> >> to clean an unknown-size memory block. >> > >> > Including a size zero memory block, yes. BLKmode was originally to do >> > things like bcopy (before modern names like memcpy were more usually >> > used), and those very much need size zero as well.h >> >> The size is possible to be zero. No asm code needs to >> be generated for "set 'const_int 0' to zero size memory"". >> stack_tie does not generate any real code. It seems ok :) >> >> While, it may not be zero size mem. This may be a concern. >> This is one reason that I would like to have an unspec_tie. > > It very much *can* be a zero size mem, that is perfectly find for > mem:BLK. There is still one concern: how to distinguish stack_tie from other insn. For example, below fake pattern: (define_insn "xx_cleanmem" [(parallel: [(set (mem:BLK (xxx)) (const_int 0)) (XXX/use "const_int_operand" "n")])]... To avoid this pattern to be recognized as 'stack_tie', 'unspec_tie' was came to mind. > >> Another reason is unspec:blk is used but various ports :) > > unspec:BLK is undefined. BLKmode is allowed on mem only. > >> >> 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0]) >> >> UNSPEC_TIE". >> >> Current patch is using this one. >> > >> > What would be the semantics of that? Just the same as the current stuff >> > I'd say, or less? It cannot be more! >> >> The semantic that I trying to achieve is "this is a special >> insn, not only a normal set to unknown size mem". > > What does that *mean*? "Special instruction"? What would what code do > for that? What would the RTL mean? > >> As you explained before on 'unspec:DI', the unspec would >> just decorate the set_src part: something DI value with >> machine-specific operation. > > An unspec is an operation on its operands, giving some (in this case) > DImode value. There is nothing special about that operation, it can be > optimised like any other, it's just not specified what exactly that > value is (to the generic compiler, the backend itself can very much > optimise stuff with it). > >> But, since 'tie_operand' is checked for this insn. >> If 'tie_operand' checks UNPSEC_TIE, then the insn >> with UNPSEC_TIE is 'a special insn'. Or interpret >> the semantic of this insn as: this insn stack_ite >> indicates "set/operate a zero size block". > > tie_operand is a predicate. The predicate of an insn has to return 1, > or the insn is not recognised. You can do the same in insn conditions > always (in principle anyway). Thank you very much for your detailed and patient explanation! BR, Jeff (Jiufu Guo) > > > Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
On Thu, Jun 15, 2023 at 03:00:40PM +0800, Jiufu Guo wrote: > >> This is the existing pattern. It may be read as an action > >> to clean an unknown-size memory block. > > > > Including a size zero memory block, yes. BLKmode was originally to do > > things like bcopy (before modern names like memcpy were more usually > > used), and those very much need size zero as well.h > > The size is possible to be zero. No asm code needs to > be generated for "set 'const_int 0' to zero size memory"". > stack_tie does not generate any real code. It seems ok :) > > While, it may not be zero size mem. This may be a concern. > This is one reason that I would like to have an unspec_tie. It very much *can* be a zero size mem, that is perfectly find for mem:BLK. > Another reason is unspec:blk is used but various ports :) unspec:BLK is undefined. BLKmode is allowed on mem only. > >> 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0]) > >> UNSPEC_TIE". > >> Current patch is using this one. > > > > What would be the semantics of that? Just the same as the current stuff > > I'd say, or less? It cannot be more! > > The semantic that I trying to achieve is "this is a special > insn, not only a normal set to unknown size mem". What does that *mean*? "Special instruction"? What would what code do for that? What would the RTL mean? > As you explained before on 'unspec:DI', the unspec would > just decorate the set_src part: something DI value with > machine-specific operation. An unspec is an operation on its operands, giving some (in this case) DImode value. There is nothing special about that operation, it can be optimised like any other, it's just not specified what exactly that value is (to the generic compiler, the backend itself can very much optimise stuff with it). > But, since 'tie_operand' is checked for this insn. > If 'tie_operand' checks UNPSEC_TIE, then the insn > with UNPSEC_TIE is 'a special insn'. Or interpret > the semantic of this insn as: this insn stack_ite > indicates "set/operate a zero size block". tie_operand is a predicate. The predicate of an insn has to return 1, or the insn is not recognised. You can do the same in insn conditions always (in principle anyway). Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, Segher Boessenkool writes: > Hi! > > On Wed, Jun 14, 2023 at 05:18:15PM +0800, Xi Ruoyao wrote: >> The generic issue here is to fix (not "papering over") the signed >> overflow, we need to perform the addition in a target machine mode. We >> may always use Pmode (IIRC const_anchor was introduced for optimizing >> some constant addresses), but can we do better? > > The main issue is that the machine description generated target code to > compute some constants, but the sanitizer treats it as if it was user > code that might do wrong things. > >> Should we try addition in both DImode and SImode for a 64-bit capable >> machine? > > Why? At least on PowerPC there is only one insn, and it is 64 bits. > The SImode version just ignores all bits other than the low 32 bits, in > both inputs and output. > >> Or should we even try more operations than addition (for eg bit >> operations like xor or shift)? Doing so will need to create a new >> target hook for const anchoring, this is the "complete rework" I meant. > Yeap! This would be a different implementation than the current const_anchor in cse.cc. In postreload.cc, there is another implementation: "reload_cse_move2add" which checks all 'add's instructions from the target. But both implementations have pros and cons. Using gcc source code as a benchmark, analyzing the relations between constants (focusing on those constants in the same function or the same basic block). IIRC, 'add's can cover most of the relations. Small part of constants can be built via other operations(e.g. shift, and, neg ,...). There may be still some benchmarks that hit other operations in the hot path. Indeed, the const_anchor feature could be enhanced to cover more cases. BR, Jeff (Jiufu Guo) > This might make const anchor useful for way more targets maybe, > including rs6000, yes :-) > > > Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, Segher Boessenkool writes: > Hi! > > On Wed, Jun 14, 2023 at 12:06:29PM +0800, Jiufu Guo wrote: >> Segher Boessenkool writes: >> I'm also thinking about other solutions: >> 1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])" >> This is the existing pattern. It may be read as an action >> to clean an unknown-size memory block. > > Including a size zero memory block, yes. BLKmode was originally to do > things like bcopy (before modern names like memcpy were more usually > used), and those very much need size zero as well.h The size is possible to be zero. No asm code needs to be generated for "set 'const_int 0' to zero size memory"". stack_tie does not generate any real code. It seems ok :) While, it may not be zero size mem. This may be a concern. This is one reason that I would like to have an unspec_tie. Another reason is unspec:blk is used but various ports :) > >> 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0]) >> UNSPEC_TIE". >> Current patch is using this one. > > What would be the semantics of that? Just the same as the current stuff > I'd say, or less? It cannot be more! The semantic that I trying to achieve is "this is a special insn, not only a normal set to unknown size mem". As you explained before on 'unspec:DI', the unspec would just decorate the set_src part: something DI value with machine-specific operation. But, since 'tie_operand' is checked for this insn. If 'tie_operand' checks UNPSEC_TIE, then the insn with UNPSEC_TIE is 'a special insn'. Or interpret the semantic of this insn as: this insn stack_ite indicates "set/operate a zero size block". Does this make sense? > >> 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0]) >> UNSPEC_TIE". >>This avoids using BLK on unspec, but using DI. > > And is incorrect because of that. > >> 4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0]) >> UNSPEC_TIE" >>There is still a mode for the unspec. > > It has VOIDmode here, which is incorrect. > >> > On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote: >> >> + && XINT (SET_SRC (set), 1) == UNSPEC_TIE >> >> + && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); >> > >> > This makes it required that the operand of an UNSPEC_TIE unspec is a >> > const_int 0. This should be documented somewhere. Ideally you would >> > want no operand at all here, but every unspec has an operand. >> >> Right! Since checked UNSPEC_TIE arleady, we may not need to check >> the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);". > > Yes. But we should write down somewhere (in a comment near the unspec > constant def for example) what the operand is -- so, "operand is usually > (const_int 0) because we have to put *something* there" or such. The > clumsiness of this is enough for me to prefer some other solution > already ;-) Thanks a lot for your comments! BR, Jeff (Jiufu Guo) > > > Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
On Wed, Jun 14, 2023 at 06:25:10PM +0200, Richard Biener wrote: > > Form rs6000.md: > > ; This is to explain that changes to the stack pointer should > > ; not be moved over loads from or stores to stack memory. > > (define_insn "stack_tie" > > That suggests it’s the hard register value that‘s protected, not the memory > pointed to. I suppose that means an unspec volatile with the reg as input > would serve the same? No? It says what it says. That is pretty vague language, of course, not entirely by accident no doubt. > Or maybe that’s not the whole story. > > > > and from rs6000-logue.cc: > > /* This ties together stack memory (MEM with an alias set of > > frame_alias_set) > > and the change to the stack pointer. */ > > static void > > rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed) > > I cannot make sense of that comment, but not sure if I really want to know … It really is the same thing: this is a bloody heavy hammer keeping the change to the stack pointer (or "hard" frame pointer) in place wrt any accesses to the stack memory. If there was a nice portable way to avoid needing this we haven't found it yet -- or a non-portable way even, and it doesn't have to be all that nice either come to think of it :-) Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi! On Wed, Jun 14, 2023 at 10:04:20AM +0100, Richard Sandiford wrote: > I'd also understood it to be either. As in, it is a may-clobber > that can be used for must-clobber. Alternatively: the value stored > is unpredictable, and can therefore be the same as the current value. Yes, it is a set with an unspecified RHS. > I think the main difference between: > > (clobber (mem:BLK …)) > > and > > (set (mem:BLK …) (unspec:BLK …)) > > is that the latter must happen for correctness (unless something > that understands the unspec proves otherwise) whereas a clobber > can validly be dropped. So for something like stack_tie, a set > seems more correct than a clobber. No, the latter can be removed as well, under exactly the same conditions: if no code after it reads what was written. This happens in branches marked dead. Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
On Wed, Jun 14, 2023 at 09:22:09AM +, Richard Biener wrote: > How can a clobber be validly dropped? Same as any other set: if no code executed after it can read whatever is written. This typically means a stack frame goes away, or simply no more code is executed *at all* after this. > For the case of stack > memory if there's no stack use after it it could be elided > and I suppose the clobber itself can be moved. But then > the function return is a stack use as well. A function return does not access the stack at all on most architectures, including PowerPC. Some epilogue insns can do, of course, but we expand to separate insns during expand already. Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
> Am 14.06.2023 um 17:41 schrieb Segher Boessenkool > : > > Hi! > >> On Wed, Jun 14, 2023 at 07:59:04AM +, Richard Biener wrote: >>> On Wed, 14 Jun 2023, Jiufu Guo wrote: >>> 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0]) >>> UNSPEC_TIE". >>> This avoids using BLK on unspec, but using DI. >> >> That gives the MEM a size which means we can interpret the (set ..) >> as killing a specific area of memory, enabling DSE of earlier >> stores. > > Or DSE can delete this tie even, if it can see some later store to the > same location without anything in between that can read what the tie > stores. > > BLKmode avoids all of this. You can call that elegant, you can call it > cheating, you can call it many things -- but it *works*. > >> AFAIU this special instruction is only supposed to prevent >> code motion (of stack memory accesses?) across this instruction? > > Form rs6000.md: > ; This is to explain that changes to the stack pointer should > ; not be moved over loads from or stores to stack memory. > (define_insn "stack_tie" That suggests it’s the hard register value that‘s protected, not the memory pointed to. I suppose that means an unspec volatile with the reg as input would serve the same? Or maybe that’s not the whole story. > and from rs6000-logue.cc: > /* This ties together stack memory (MEM with an alias set of frame_alias_set) > and the change to the stack pointer. */ > static void > rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed) I cannot make sense of that comment, but not sure if I really want to know … > A big reason this is needed is because of all the hard frame pointer > stuff, which the generic parts of GCC require, but there is no register > for that in the Power architecture. Nothing is an issue here in most > cases, but sometimes we need to do unusual things to the stack, say for > alloca. > >> I'd say a >> >> (may_clobber (mem:BLK (reg:DI 1 1))) > > "clobber" always means "may clobber". (clobber X) means X is written > with some unspecified value, which may well be whatever value it > currently holds. Via some magical means or whatever, there is no > mechanism specified, just the effects :-) > >> might be more to the point? I've used "may_clobber" which doesn't >> exist since I'm not sure whether a clobber is considered a kill. >> The docs say "Represents the storing or possible storing of an >> unpredictable..." - what is it? Storing or possible storing? > > It is the same thing. "clobber" means the same thing as "set", except > the value that is written is not specified. > >> I suppose stack_tie should be less strict than the documented >> (clobber (mem:BLK (const_int 0))) (clobber all memory). > > "clobber" is nicer than the set to (const_int 0). Does it work though? > All this code is always fragile :-/ I'm all for this change, don't get > me wrong, but preferably things stay in working order. > > We use "stack_tie" as a last resort heavy hammer anyway, in all normal > cases we explain the actual data flow explicitly and correctly, also > between the various registers used in the *logues. > > > Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi! On Wed, Jun 14, 2023 at 09:52:37AM +, Richard Biener wrote: > I see. So > > (parallel > (unspec stack_tie) > (clobber (mem:BLK ...))) Written like this, without a "set", *every* unspec has to be an unspec_volatile, for the same reason as all inline asms without outputs always are considered volatile asm. The "unspec" arm of the parallel can be omitted, and if that is valid RTL (possibly after other changes, like omitting the parallel,replacing it by its one remaining arm), this is a prefectly valid transformation. > I suppose it needs to be an unspec_volatile? It feels like > the stack_ties are a delicate hack preventing enough but not too > much optimization ... Yes. So let's please not disturb it :-) It isn't a "delicate" hack I would say, but its effects are needed in some places, and messing it up leads to hard to debug problems. Which had happened time and time again over the years. It just is hard to deal with variable sized stack adjustments and the like. As long as we only use stack ties in such unusual cases, all is fine. There are worse things, like what we have the frame_pointer_needed_indeed thing for :-) Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi! On Wed, Jun 14, 2023 at 05:26:52PM +0800, Jiufu Guo wrote: > Richard Biener writes: > >> 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0]) > >> UNSPEC_TIE". > >>This avoids using BLK on unspec, but using DI. > > > > That gives the MEM a size which means we can interpret the (set ..) > > as killing a specific area of memory, enabling DSE of earlier > > stores. > > Oh, thanks! > While with 'unspec:DI', I'm wondering if it means this 'set' would > do some special things other than pure 'set' to the memory. No, that is not what unspec means. It just means "some DImode value I'm not telling you anything about". If to get that value there is some important work done (that should not be oprimised away, say) you need unspec_volatile, which means just that: there is an unspecified side effect done by that insn, so it has to be done on the real machine exactly like on the abstract C machine, so the insn has big restrictions on being moved and removed etc. We can replace the RHS of (almost) *every* set with an unspec, and the compiler would still work, just would generate pretty lousy code. But at least CSE and DSE (and everything else purely dataflow) would still work :-) Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi! On Wed, Jun 14, 2023 at 07:59:04AM +, Richard Biener wrote: > On Wed, 14 Jun 2023, Jiufu Guo wrote: > > 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0]) > > UNSPEC_TIE". > >This avoids using BLK on unspec, but using DI. > > That gives the MEM a size which means we can interpret the (set ..) > as killing a specific area of memory, enabling DSE of earlier > stores. Or DSE can delete this tie even, if it can see some later store to the same location without anything in between that can read what the tie stores. BLKmode avoids all of this. You can call that elegant, you can call it cheating, you can call it many things -- but it *works*. > AFAIU this special instruction is only supposed to prevent > code motion (of stack memory accesses?) across this instruction? Form rs6000.md: ; This is to explain that changes to the stack pointer should ; not be moved over loads from or stores to stack memory. (define_insn "stack_tie" and from rs6000-logue.cc: /* This ties together stack memory (MEM with an alias set of frame_alias_set) and the change to the stack pointer. */ static void rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed) A big reason this is needed is because of all the hard frame pointer stuff, which the generic parts of GCC require, but there is no register for that in the Power architecture. Nothing is an issue here in most cases, but sometimes we need to do unusual things to the stack, say for alloca. > I'd say a > > (may_clobber (mem:BLK (reg:DI 1 1))) "clobber" always means "may clobber". (clobber X) means X is written with some unspecified value, which may well be whatever value it currently holds. Via some magical means or whatever, there is no mechanism specified, just the effects :-) > might be more to the point? I've used "may_clobber" which doesn't > exist since I'm not sure whether a clobber is considered a kill. > The docs say "Represents the storing or possible storing of an > unpredictable..." - what is it? Storing or possible storing? It is the same thing. "clobber" means the same thing as "set", except the value that is written is not specified. > I suppose stack_tie should be less strict than the documented > (clobber (mem:BLK (const_int 0))) (clobber all memory). "clobber" is nicer than the set to (const_int 0). Does it work though? All this code is always fragile :-/ I'm all for this change, don't get me wrong, but preferably things stay in working order. We use "stack_tie" as a last resort heavy hammer anyway, in all normal cases we explain the actual data flow explicitly and correctly, also between the various registers used in the *logues. Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi! On Wed, Jun 14, 2023 at 12:06:29PM +0800, Jiufu Guo wrote: > Segher Boessenkool writes: > I'm also thinking about other solutions: > 1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])" > This is the existing pattern. It may be read as an action > to clean an unknown-size memory block. Including a size zero memory block, yes. BLKmode was originally to do things like bcopy (before modern names like memcpy were more usually used), and those very much need size zero as well. > 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0]) > UNSPEC_TIE". > Current patch is using this one. What would be the semantics of that? Just the same as the current stuff I'd say, or less? It cannot be more! > 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0]) > UNSPEC_TIE". >This avoids using BLK on unspec, but using DI. And is incorrect because of that. > 4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0]) > UNSPEC_TIE" >There is still a mode for the unspec. It has VOIDmode here, which is incorrect. > > On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote: > >> +&& XINT (SET_SRC (set), 1) == UNSPEC_TIE > >> +&& XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); > > > > This makes it required that the operand of an UNSPEC_TIE unspec is a > > const_int 0. This should be documented somewhere. Ideally you would > > want no operand at all here, but every unspec has an operand. > > Right! Since checked UNSPEC_TIE arleady, we may not need to check > the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);". Yes. But we should write down somewhere (in a comment near the unspec constant def for example) what the operand is -- so, "operand is usually (const_int 0) because we have to put *something* there" or such. The clumsiness of this is enough for me to prefer some other solution already ;-) Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi! On Wed, Jun 14, 2023 at 05:18:15PM +0800, Xi Ruoyao wrote: > The generic issue here is to fix (not "papering over") the signed > overflow, we need to perform the addition in a target machine mode. We > may always use Pmode (IIRC const_anchor was introduced for optimizing > some constant addresses), but can we do better? The main issue is that the machine description generated target code to compute some constants, but the sanitizer treats it as if it was user code that might do wrong things. > Should we try addition in both DImode and SImode for a 64-bit capable > machine? Why? At least on PowerPC there is only one insn, and it is 64 bits. The SImode version just ignores all bits other than the low 32 bits, in both inputs and output. > Or should we even try more operations than addition (for eg bit > operations like xor or shift)? Doing so will need to create a new > target hook for const anchoring, this is the "complete rework" I meant. This might make const anchor useful for way more targets maybe, including rs6000, yes :-) Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Richard Biener writes: > On Wed, 14 Jun 2023, Richard Sandiford wrote: > >> Richard Biener writes: >> > On Wed, 14 Jun 2023, Richard Sandiford wrote: >> > >> >> Richard Biener writes: >> >> > AFAIU this special instruction is only supposed to prevent >> >> > code motion (of stack memory accesses?) across this instruction? >> >> > I'd say a >> >> > >> >> > (may_clobber (mem:BLK (reg:DI 1 1))) >> >> > >> >> > might be more to the point? I've used "may_clobber" which doesn't >> >> > exist since I'm not sure whether a clobber is considered a kill. >> >> > The docs say "Represents the storing or possible storing of an >> >> > unpredictable..." - what is it? Storing or possible storing? >> >> >> >> I'd also understood it to be either. As in, it is a may-clobber >> >> that can be used for must-clobber. Alternatively: the value stored >> >> is unpredictable, and can therefore be the same as the current value. >> >> >> >> I think the main difference between: >> >> >> >> (clobber (mem:BLK ?)) >> >> >> >> and >> >> >> >> (set (mem:BLK ?) (unspec:BLK ?)) >> >> >> >> is that the latter must happen for correctness (unless something >> >> that understands the unspec proves otherwise) whereas a clobber >> >> can validly be dropped. So for something like stack_tie, a set >> >> seems more correct than a clobber. >> > >> > How can a clobber be validly dropped? For the case of stack >> > memory if there's no stack use after it it could be elided >> > and I suppose the clobber itself can be moved. But then >> > the function return is a stack use as well. >> > >> > Btw, with the same reason the (set (mem:...)) could be removed, no? >> > Or is the (unspec:) SET_SRC having implicit side-effects that >> > prevents the removal (so rs6000 could have its stack_tie removed)? >> > >> > That said, I fail to see how a clobber is special here. >> >> Clobbers are for side-effects. They don't start a def-use chain. >> E.g. any use after a full clobber is an uninitialised read rather >> than a read of the clobber ?result?. > > I see. So > > (parallel > (unspec stack_tie) > (clobber (mem:BLK ...))) > > then? I suppose it needs to be an unspec_volatile? Yeah, it would need to be unspec_volatile, at which point it becomes quite a big hammer. > It feels like the stack_ties are a delicate hack preventing enough but > not too much optimization ... Yup. I think the only non-hacky way would be to have dedicated RTL for memory becoming valid and becoming invalid. Anything else is a compromise. But TBH, I still think the (set (mem:BLK …) (unspec:BLK …)) strikes the right balance, unless there's a specific argument otherwise. The effect on memory isn't a side effect (contrary to what clobber implies) but instead is the main purpose of allocating and deallocating stack memory. Thanks, Richard
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
On Wed, 14 Jun 2023, Richard Sandiford wrote: > Richard Biener writes: > > On Wed, 14 Jun 2023, Richard Sandiford wrote: > > > >> Richard Biener writes: > >> > AFAIU this special instruction is only supposed to prevent > >> > code motion (of stack memory accesses?) across this instruction? > >> > I'd say a > >> > > >> > (may_clobber (mem:BLK (reg:DI 1 1))) > >> > > >> > might be more to the point? I've used "may_clobber" which doesn't > >> > exist since I'm not sure whether a clobber is considered a kill. > >> > The docs say "Represents the storing or possible storing of an > >> > unpredictable..." - what is it? Storing or possible storing? > >> > >> I'd also understood it to be either. As in, it is a may-clobber > >> that can be used for must-clobber. Alternatively: the value stored > >> is unpredictable, and can therefore be the same as the current value. > >> > >> I think the main difference between: > >> > >> (clobber (mem:BLK ?)) > >> > >> and > >> > >> (set (mem:BLK ?) (unspec:BLK ?)) > >> > >> is that the latter must happen for correctness (unless something > >> that understands the unspec proves otherwise) whereas a clobber > >> can validly be dropped. So for something like stack_tie, a set > >> seems more correct than a clobber. > > > > How can a clobber be validly dropped? For the case of stack > > memory if there's no stack use after it it could be elided > > and I suppose the clobber itself can be moved. But then > > the function return is a stack use as well. > > > > Btw, with the same reason the (set (mem:...)) could be removed, no? > > Or is the (unspec:) SET_SRC having implicit side-effects that > > prevents the removal (so rs6000 could have its stack_tie removed)? > > > > That said, I fail to see how a clobber is special here. > > Clobbers are for side-effects. They don't start a def-use chain. > E.g. any use after a full clobber is an uninitialised read rather > than a read of the clobber ?result?. I see. So (parallel (unspec stack_tie) (clobber (mem:BLK ...))) then? I suppose it needs to be an unspec_volatile? It feels like the stack_ties are a delicate hack preventing enough but not too much optimization ... > In contrast, a set of memory with an unspec source is in dataflow terms > the same as a set of memory with a specified source. (some unspecs > actually have well-defined values, it's just that only the target code > knows what those well-defined value are.) > > So a set of memory could only be removed if DSE proves that there are no > reads of the set bytes before the next set(s) to the same bytes of memory. > And memory is always live. > > Thanks, > Richard > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Richard Biener writes: > On Wed, 14 Jun 2023, Richard Sandiford wrote: > >> Richard Biener writes: >> > AFAIU this special instruction is only supposed to prevent >> > code motion (of stack memory accesses?) across this instruction? >> > I'd say a >> > >> > (may_clobber (mem:BLK (reg:DI 1 1))) >> > >> > might be more to the point? I've used "may_clobber" which doesn't >> > exist since I'm not sure whether a clobber is considered a kill. >> > The docs say "Represents the storing or possible storing of an >> > unpredictable..." - what is it? Storing or possible storing? >> >> I'd also understood it to be either. As in, it is a may-clobber >> that can be used for must-clobber. Alternatively: the value stored >> is unpredictable, and can therefore be the same as the current value. >> >> I think the main difference between: >> >> (clobber (mem:BLK ?)) >> >> and >> >> (set (mem:BLK ?) (unspec:BLK ?)) >> >> is that the latter must happen for correctness (unless something >> that understands the unspec proves otherwise) whereas a clobber >> can validly be dropped. So for something like stack_tie, a set >> seems more correct than a clobber. > > How can a clobber be validly dropped? For the case of stack > memory if there's no stack use after it it could be elided > and I suppose the clobber itself can be moved. But then > the function return is a stack use as well. > > Btw, with the same reason the (set (mem:...)) could be removed, no? > Or is the (unspec:) SET_SRC having implicit side-effects that > prevents the removal (so rs6000 could have its stack_tie removed)? > > That said, I fail to see how a clobber is special here. Clobbers are for side-effects. They don't start a def-use chain. E.g. any use after a full clobber is an uninitialised read rather than a read of the clobber “result”. In contrast, a set of memory with an unspec source is in dataflow terms the same as a set of memory with a specified source. (some unspecs actually have well-defined values, it's just that only the target code knows what those well-defined value are.) So a set of memory could only be removed if DSE proves that there are no reads of the set bytes before the next set(s) to the same bytes of memory. And memory is always live. Thanks, Richard
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, Richard Sandiford writes: > Richard Biener writes: >> AFAIU this special instruction is only supposed to prevent >> code motion (of stack memory accesses?) across this instruction? >> I'd say a >> >> (may_clobber (mem:BLK (reg:DI 1 1))) >> >> might be more to the point? I've used "may_clobber" which doesn't >> exist since I'm not sure whether a clobber is considered a kill. >> The docs say "Represents the storing or possible storing of an >> unpredictable..." - what is it? Storing or possible storing? > > I'd also understood it to be either. As in, it is a may-clobber > that can be used for must-clobber. Alternatively: the value stored > is unpredictable, and can therefore be the same as the current value. > > I think the main difference between: > > (clobber (mem:BLK …)) > > and > > (set (mem:BLK …) (unspec:BLK …)) > > is that the latter must happen for correctness (unless something > that understands the unspec proves otherwise) whereas a clobber > can validly be dropped. So for something like stack_tie, a set > seems more correct than a clobber. Thanks a lot for all your helpful comments! BR, Jeff (Jiufu Guo) > > Thanks, > Richard
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, Richard Biener writes: > On Wed, 14 Jun 2023, Jiufu Guo wrote: > >> >> Hi, >> >> Segher Boessenkool writes: >> >> > Hi! >> > >> > As I said in a reply to the original patch: not okay. Sorry. >> >> Thanks a lot for your comments! >> I'm also thinking about other solutions: >> 1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])" >> This is the existing pattern. It may be read as an action >> to clean an unknown-size memory block. >> >> 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0]) >> UNSPEC_TIE". >> Current patch is using this one. >> >> 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0]) >> UNSPEC_TIE". >>This avoids using BLK on unspec, but using DI. > > That gives the MEM a size which means we can interpret the (set ..) > as killing a specific area of memory, enabling DSE of earlier > stores. Oh, thanks! While with 'unspec:DI', I'm wondering if it means this 'set' would do some special things other than pure 'set' to the memory. BR, Jeff (Jiufu Guo) > > AFAIU this special instruction is only supposed to prevent > code motion (of stack memory accesses?) across this instruction? > I'd say a > > (may_clobber (mem:BLK (reg:DI 1 1))) > > might be more to the point? I've used "may_clobber" which doesn't > exist since I'm not sure whether a clobber is considered a kill. > The docs say "Represents the storing or possible storing of an > unpredictable..." - what is it? Storing or possible storing? > I suppose stack_tie should be less strict than the documented > (clobber (mem:BLK (const_int 0))) (clobber all memory). > > ? > >> 4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0]) >> UNSPEC_TIE" >>There is still a mode for the unspec. >> >> >> > >> > But some comments on this patch: >> > >> > On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote: >> >> + && XINT (SET_SRC (set), 1) == UNSPEC_TIE >> >> + && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); >> > >> > This makes it required that the operand of an UNSPEC_TIE unspec is a >> > const_int 0. This should be documented somewhere. Ideally you would >> > want no operand at all here, but every unspec has an operand. >> >> Right! Since checked UNSPEC_TIE arleady, we may not need to check >> the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);". >> >> > >> >> + RTVEC_ELT (p, i) >> >> + = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), >> >> + UNSPEC_TIE)); >> > >> > If it is hard to indent your code, your code is trying to do to much. >> > Just have an extra temporary? >> > >> > rtx un = gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), >> > UNSPEC_TIE); >> > RTVEC_ELT (p, i) = gen_rtx_SET (mem, un); >> > >> > That is shorter even, and certainly more readable :-) >> >> Yeap, thanks! >> >> > >> >> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block" >> >>operands[4] = gen_frame_mem (Pmode, operands[1]); >> >>p = rtvec_alloc (1); >> >>RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), >> >> - const0_rtx); >> >> + gen_rtx_UNSPEC (BLKmode, >> >> + gen_rtvec (1, const0_rtx), >> >> + UNSPEC_TIE)); >> >>operands[5] = gen_rtx_PARALLEL (VOIDmode, p); >> > >> > I have a hard time to see how this could ever be seen as clearer or more >> > obvious or anything like that :-( >> >> I was thinking about just invoking gen_stack_tie here. >> >> BR, >> Jeff (Jiufu Guo) >> >> > >> > >> > Segher >>
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
On Wed, 14 Jun 2023, Richard Sandiford wrote: > Richard Biener writes: > > AFAIU this special instruction is only supposed to prevent > > code motion (of stack memory accesses?) across this instruction? > > I'd say a > > > > (may_clobber (mem:BLK (reg:DI 1 1))) > > > > might be more to the point? I've used "may_clobber" which doesn't > > exist since I'm not sure whether a clobber is considered a kill. > > The docs say "Represents the storing or possible storing of an > > unpredictable..." - what is it? Storing or possible storing? > > I'd also understood it to be either. As in, it is a may-clobber > that can be used for must-clobber. Alternatively: the value stored > is unpredictable, and can therefore be the same as the current value. > > I think the main difference between: > > (clobber (mem:BLK ?)) > > and > > (set (mem:BLK ?) (unspec:BLK ?)) > > is that the latter must happen for correctness (unless something > that understands the unspec proves otherwise) whereas a clobber > can validly be dropped. So for something like stack_tie, a set > seems more correct than a clobber. How can a clobber be validly dropped? For the case of stack memory if there's no stack use after it it could be elided and I suppose the clobber itself can be moved. But then the function return is a stack use as well. Btw, with the same reason the (set (mem:...)) could be removed, no? Or is the (unspec:) SET_SRC having implicit side-effects that prevents the removal (so rs6000 could have its stack_tie removed)? That said, I fail to see how a clobber is special here. Richard. > Thanks, > Richard > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
On Wed, 2023-06-14 at 09:55 +0800, Jiufu Guo wrote: > Hi, > > Xi Ruoyao writes: > > > On Tue, 2023-06-13 at 20:23 +0800, Jiufu Guo via Gcc-patches wrote: > > > > > Compare with previous version, this addes ChangeLog and removes > > > const_anchor parts. > > > https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621356.html. > > > > [Off topic] > > > > const_anchor is just broken now. See > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104843 and the thread > > beginning at > > https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591470.html. If > > you want to use it for rs6000 I guess you need to fix it first... > > Thanks so much for pointing out this. It seems about supporting > negative value, right? > > As you say: for 1. "g(0x8123, 0x81240001)", it would be fine. > > The generated insns are: > (insn 5 2 6 2 (set (reg:DI 117) > (const_int -2128347135 [0x81240001])) "negative.c":5:3 681 > {*movdi_internal64} > (nil)) > (insn 6 5 7 2 (set (reg:DI 118) > (plus:DI (reg:DI 117) > (const_int -2 [0xfffe]))) "negative.c":5:3 66 > {*adddi3} > (expr_list:REG_EQUAL (const_int -2128347137 [0x8123]) > (nil))) > > While for 2. "g (0x7fff, 0x8001)", the generated rtl insns: > (insn 5 2 6 2 (set (reg:DI 117) > (const_int -2147483647 [0x8001])) "negative.c":5:3 681 > {*movdi_internal64} > (nil)) > (insn 7 6 8 2 (set (reg:DI 3 3) > (const_int 2147483647 [0x7fff])) "negative.c":5:3 681 > {*movdi_internal64} > (nil)) > > The current const_anchor does not generate sth like: "r3 = r117 - 2" > But I would lean to say it is the limitation of current implementation: > "0x8001" and "0x7fff" hit different anchors(even these > two values are 'close' on some aspect.) The generic issue here is to fix (not "papering over") the signed overflow, we need to perform the addition in a target machine mode. We may always use Pmode (IIRC const_anchor was introduced for optimizing some constant addresses), but can we do better? Should we try addition in both DImode and SImode for a 64-bit capable machine? Or should we even try more operations than addition (for eg bit operations like xor or shift)? Doing so will need to create a new target hook for const anchoring, this is the "complete rework" I meant. -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Richard Biener writes: > AFAIU this special instruction is only supposed to prevent > code motion (of stack memory accesses?) across this instruction? > I'd say a > > (may_clobber (mem:BLK (reg:DI 1 1))) > > might be more to the point? I've used "may_clobber" which doesn't > exist since I'm not sure whether a clobber is considered a kill. > The docs say "Represents the storing or possible storing of an > unpredictable..." - what is it? Storing or possible storing? I'd also understood it to be either. As in, it is a may-clobber that can be used for must-clobber. Alternatively: the value stored is unpredictable, and can therefore be the same as the current value. I think the main difference between: (clobber (mem:BLK …)) and (set (mem:BLK …) (unspec:BLK …)) is that the latter must happen for correctness (unless something that understands the unspec proves otherwise) whereas a clobber can validly be dropped. So for something like stack_tie, a set seems more correct than a clobber. Thanks, Richard
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
On Wed, 14 Jun 2023, Jiufu Guo wrote: > > Hi, > > Segher Boessenkool writes: > > > Hi! > > > > As I said in a reply to the original patch: not okay. Sorry. > > Thanks a lot for your comments! > I'm also thinking about other solutions: > 1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])" > This is the existing pattern. It may be read as an action > to clean an unknown-size memory block. > > 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0]) > UNSPEC_TIE". > Current patch is using this one. > > 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0]) > UNSPEC_TIE". >This avoids using BLK on unspec, but using DI. That gives the MEM a size which means we can interpret the (set ..) as killing a specific area of memory, enabling DSE of earlier stores. AFAIU this special instruction is only supposed to prevent code motion (of stack memory accesses?) across this instruction? I'd say a (may_clobber (mem:BLK (reg:DI 1 1))) might be more to the point? I've used "may_clobber" which doesn't exist since I'm not sure whether a clobber is considered a kill. The docs say "Represents the storing or possible storing of an unpredictable..." - what is it? Storing or possible storing? I suppose stack_tie should be less strict than the documented (clobber (mem:BLK (const_int 0))) (clobber all memory). ? > 4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0]) > UNSPEC_TIE" >There is still a mode for the unspec. > > > > > > But some comments on this patch: > > > > On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote: > >> +&& XINT (SET_SRC (set), 1) == UNSPEC_TIE > >> +&& XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); > > > > This makes it required that the operand of an UNSPEC_TIE unspec is a > > const_int 0. This should be documented somewhere. Ideally you would > > want no operand at all here, but every unspec has an operand. > > Right! Since checked UNSPEC_TIE arleady, we may not need to check > the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);". > > > > >> + RTVEC_ELT (p, i) > >> + = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), > >> + UNSPEC_TIE)); > > > > If it is hard to indent your code, your code is trying to do to much. > > Just have an extra temporary? > > > > rtx un = gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), > > UNSPEC_TIE); > > RTVEC_ELT (p, i) = gen_rtx_SET (mem, un); > > > > That is shorter even, and certainly more readable :-) > > Yeap, thanks! > > > > >> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block" > >>operands[4] = gen_frame_mem (Pmode, operands[1]); > >>p = rtvec_alloc (1); > >>RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), > >> -const0_rtx); > >> +gen_rtx_UNSPEC (BLKmode, > >> +gen_rtvec (1, const0_rtx), > >> +UNSPEC_TIE)); > >>operands[5] = gen_rtx_PARALLEL (VOIDmode, p); > > > > I have a hard time to see how this could ever be seen as clearer or more > > obvious or anything like that :-( > > I was thinking about just invoking gen_stack_tie here. > > BR, > Jeff (Jiufu Guo) > > > > > > > Segher > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, Segher Boessenkool writes: > Hi! > > As I said in a reply to the original patch: not okay. Sorry. Thanks a lot for your comments! I'm also thinking about other solutions: 1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])" This is the existing pattern. It may be read as an action to clean an unknown-size memory block. 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0]) UNSPEC_TIE". Current patch is using this one. 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0]) UNSPEC_TIE". This avoids using BLK on unspec, but using DI. 4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0]) UNSPEC_TIE" There is still a mode for the unspec. > > But some comments on this patch: > > On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote: >> + && XINT (SET_SRC (set), 1) == UNSPEC_TIE >> + && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); > > This makes it required that the operand of an UNSPEC_TIE unspec is a > const_int 0. This should be documented somewhere. Ideally you would > want no operand at all here, but every unspec has an operand. Right! Since checked UNSPEC_TIE arleady, we may not need to check the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);". > >> + RTVEC_ELT (p, i) >> += gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), >> +UNSPEC_TIE)); > > If it is hard to indent your code, your code is trying to do to much. > Just have an extra temporary? > > rtx un = gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), > UNSPEC_TIE); > RTVEC_ELT (p, i) = gen_rtx_SET (mem, un); > > That is shorter even, and certainly more readable :-) Yeap, thanks! > >> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block" >>operands[4] = gen_frame_mem (Pmode, operands[1]); >>p = rtvec_alloc (1); >>RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), >> - const0_rtx); >> + gen_rtx_UNSPEC (BLKmode, >> + gen_rtvec (1, const0_rtx), >> + UNSPEC_TIE)); >>operands[5] = gen_rtx_PARALLEL (VOIDmode, p); > > I have a hard time to see how this could ever be seen as clearer or more > obvious or anything like that :-( I was thinking about just invoking gen_stack_tie here. BR, Jeff (Jiufu Guo) > > > Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi Segher, David, David Edelsohn writes: > On Tue, Jun 13, 2023 at 2:16 PM Segher Boessenkool > wrote: >> >> Hi! >> >> On Tue, Jun 13, 2023 at 10:15:49AM +0800, Jiufu Guo wrote: >> > David Edelsohn writes: >> > > >> > > This definitely seems to be a better solution. >> > > >> > > The TARGET_CONST_ANCHOR change should not be part of this patch. Also >> > > there is no ChangeLog for the patch. >> > >> > Thanks a lot for your quick review!! And sorry for the sending this patch >> > in a hurry. I would update the patch accordingly. >> >> > > This generally looks correct and consistent with other ports. I want >> > > to give Segher a chance to double check it, if he wishes. >> >> The documentation is very clear that the only thing for which you can >> have BLKmode is "mem". Not unspec, only "mem". >> >> Let's not do this. The existing code has clear and obvious semantics, >> which is documented as well -- there is no reason to make it worse in >> every respect. Thanks for all your insight comments! Yeap, while "unspec:BLK" is very widely used already on various ports. And it seems a few place is using BLKmode without strictly align with the document :( It would not be very good thing, but maybe no better solutions. For existing code "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])" Since it is a set, the operand set_src should be valid for the mode of the set_dest. While set_src is 'const_int 0'. And this 'set' may be mis-readed as 'a memory is zeroed' or 'no-op to a mem'. Using unspec here would just say this is an special operation instead a normal 'const_int 0'. BR, Jeff (Jiufu Guo) > > Segher, > > Unfortunately, GCC now is inconsistent and this response is incorrect. > The documentation is out of date or was ignored and the "facts on the > ground" contradict your review. > > Yes, (const_int 0) is supposed to be a general no-op and BLKmode only > is supposed to be used for MEM, but other major targets (arm, aarch64, > riscv, s390) all use unspec:BLK and specifically UNSPEC_TIE. rs6000 > is the only port that does not follow this convention. The middle-end > has adapted to the behavior of all of the other targets, whether that > conformed to the documentation or not. The rs6000 port needs to be > fixed and Jiufu's approach is the correct one, consistent with all > other targets for stack tie. If the documentation differs, the > documentation needs to be updated, not a different approach for the > rs6000 port. Jiufu's patch is correct. > > Thanks, David
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, Xi Ruoyao writes: > On Tue, 2023-06-13 at 20:23 +0800, Jiufu Guo via Gcc-patches wrote: > >> Compare with previous version, this addes ChangeLog and removes >> const_anchor parts. >> https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621356.html. > > [Off topic] > > const_anchor is just broken now. See > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104843 and the thread > beginning at > https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591470.html. If > you want to use it for rs6000 I guess you need to fix it first... Thanks so much for pointing out this. It seems about supporting negative value, right? As you say: for 1. "g(0x8123, 0x81240001)", it would be fine. The generated insns are: (insn 5 2 6 2 (set (reg:DI 117) (const_int -2128347135 [0x81240001])) "negative.c":5:3 681 {*movdi_internal64} (nil)) (insn 6 5 7 2 (set (reg:DI 118) (plus:DI (reg:DI 117) (const_int -2 [0xfffe]))) "negative.c":5:3 66 {*adddi3} (expr_list:REG_EQUAL (const_int -2128347137 [0x8123]) (nil))) While for 2. "g (0x7fff, 0x8001)", the generated rtl insns: (insn 5 2 6 2 (set (reg:DI 117) (const_int -2147483647 [0x8001])) "negative.c":5:3 681 {*movdi_internal64} (nil)) (insn 7 6 8 2 (set (reg:DI 3 3) (const_int 2147483647 [0x7fff])) "negative.c":5:3 681 {*movdi_internal64} (nil)) The current const_anchor does not generate sth like: "r3 = r117 - 2" But I would lean to say it is the limitation of current implementation: "0x8001" and "0x7fff" hit different anchors(even these two values are 'close' on some aspect.) BR, Jeff (Jiufu Guo) > > To me const_anchor needs a complete rework but I don't want to spend my > time on it.
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
On Tue, Jun 13, 2023 at 2:16 PM Segher Boessenkool wrote: > > Hi! > > On Tue, Jun 13, 2023 at 10:15:49AM +0800, Jiufu Guo wrote: > > David Edelsohn writes: > > > > > > This definitely seems to be a better solution. > > > > > > The TARGET_CONST_ANCHOR change should not be part of this patch. Also > > > there is no ChangeLog for the patch. > > > > Thanks a lot for your quick review!! And sorry for the sending this patch > > in a hurry. I would update the patch accordingly. > > > > This generally looks correct and consistent with other ports. I want > > > to give Segher a chance to double check it, if he wishes. > > The documentation is very clear that the only thing for which you can > have BLKmode is "mem". Not unspec, only "mem". > > Let's not do this. The existing code has clear and obvious semantics, > which is documented as well -- there is no reason to make it worse in > every respect. Segher, Unfortunately, GCC now is inconsistent and this response is incorrect. The documentation is out of date or was ignored and the "facts on the ground" contradict your review. Yes, (const_int 0) is supposed to be a general no-op and BLKmode only is supposed to be used for MEM, but other major targets (arm, aarch64, riscv, s390) all use unspec:BLK and specifically UNSPEC_TIE. rs6000 is the only port that does not follow this convention. The middle-end has adapted to the behavior of all of the other targets, whether that conformed to the documentation or not. The rs6000 port needs to be fixed and Jiufu's approach is the correct one, consistent with all other targets for stack tie. If the documentation differs, the documentation needs to be updated, not a different approach for the rs6000 port. Jiufu's patch is correct. Thanks, David
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi! As I said in a reply to the original patch: not okay. Sorry. But some comments on this patch: On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote: > + && XINT (SET_SRC (set), 1) == UNSPEC_TIE > + && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); This makes it required that the operand of an UNSPEC_TIE unspec is a const_int 0. This should be documented somewhere. Ideally you would want no operand at all here, but every unspec has an operand. > + RTVEC_ELT (p, i) > + = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), > + UNSPEC_TIE)); If it is hard to indent your code, your code is trying to do to much. Just have an extra temporary? rtx un = gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), UNSPEC_TIE); RTVEC_ELT (p, i) = gen_rtx_SET (mem, un); That is shorter even, and certainly more readable :-) > @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block" >operands[4] = gen_frame_mem (Pmode, operands[1]); >p = rtvec_alloc (1); >RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), > - const0_rtx); > + gen_rtx_UNSPEC (BLKmode, > + gen_rtvec (1, const0_rtx), > + UNSPEC_TIE)); >operands[5] = gen_rtx_PARALLEL (VOIDmode, p); I have a hard time to see how this could ever be seen as clearer or more obvious or anything like that :-( Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi! On Tue, Jun 13, 2023 at 10:15:49AM +0800, Jiufu Guo wrote: > David Edelsohn writes: > > > > This definitely seems to be a better solution. > > > > The TARGET_CONST_ANCHOR change should not be part of this patch. Also > > there is no ChangeLog for the patch. > > Thanks a lot for your quick review!! And sorry for the sending this patch > in a hurry. I would update the patch accordingly. > > This generally looks correct and consistent with other ports. I want > > to give Segher a chance to double check it, if he wishes. The documentation is very clear that the only thing for which you can have BLKmode is "mem". Not unspec, only "mem". Let's not do this. The existing code has clear and obvious semantics, which is documented as well -- there is no reason to make it worse in every respect! Segher
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
On Tue, 2023-06-13 at 20:23 +0800, Jiufu Guo via Gcc-patches wrote: > Compare with previous version, this addes ChangeLog and removes > const_anchor parts. > https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621356.html. [Off topic] const_anchor is just broken now. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104843 and the thread beginning at https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591470.html. If you want to use it for rs6000 I guess you need to fix it first... To me const_anchor needs a complete rework but I don't want to spend my time on it. -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
[PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, For stack_tie, currently below insn is generated: (insn 15 14 16 3 (parallel [ (set (mem/c:BLK (reg/f:DI 1 1) [1 A8]) (const_int 0 [0])) ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie} (nil)) It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])". This maybe looks like "a memory block is zerored", while actually stack_tie may be more like a placeholder, and does not generate any thing. To avoid potential misunderstand, "UNPSEC:BLK [(const_int 0)].." could be used here. Compare with previous version, this addes ChangeLog and removes const_anchor parts. https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621356.html. Bootstrap®test pass on ppc64{,le}. Is this ok for trunk? BR, Jeff (Jiufu Guo) gcc/ChangeLog: * config/rs6000/predicates.md (tie_operand): Update to match new stack_tie pattern. * config/rs6000/rs6000-logue.cc (rs6000_emit_stack_tie): Update to use the new stack_tie pattern. * config/rs6000/rs6000.md (UNSPEC_TIE): New UNSPEC. (restore_stack_block): Update to use the new stack_tie pattern. (restore_stack_nonlocal): Likewise. (stack_tie): Update pattern to use UNSPEC_TIE. (stack_restore_tie): Likewise. --- gcc/config/rs6000/predicates.md | 11 +++ gcc/config/rs6000/rs6000-logue.cc | 4 +++- gcc/config/rs6000/rs6000.md | 14 ++ 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md index a16ee30f0c0..4748cb37ce8 100644 --- a/gcc/config/rs6000/predicates.md +++ b/gcc/config/rs6000/predicates.md @@ -1854,10 +1854,13 @@ (define_predicate "stmw_operation" (define_predicate "tie_operand" (match_code "parallel") { - return (GET_CODE (XVECEXP (op, 0, 0)) == SET - && MEM_P (XEXP (XVECEXP (op, 0, 0), 0)) - && GET_MODE (XEXP (XVECEXP (op, 0, 0), 0)) == BLKmode - && XEXP (XVECEXP (op, 0, 0), 1) == const0_rtx); + rtx set = XVECEXP (op, 0, 0); + return (GET_CODE (set) == SET + && MEM_P (SET_DEST (set)) + && GET_MODE (SET_DEST (set)) == BLKmode + && GET_CODE (SET_SRC (set)) == UNSPEC + && XINT (SET_SRC (set), 1) == UNSPEC_TIE + && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); }) ;; Match a small code model toc reference (or medium and large diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc index bc6b153b59f..b99f43a8282 100644 --- a/gcc/config/rs6000/rs6000-logue.cc +++ b/gcc/config/rs6000/rs6000-logue.cc @@ -1463,7 +1463,9 @@ rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed) while (--i >= 0) { rtx mem = gen_frame_mem (BLKmode, regs[i]); - RTVEC_ELT (p, i) = gen_rtx_SET (mem, const0_rtx); + RTVEC_ELT (p, i) + = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), + UNSPEC_TIE)); } emit_insn (gen_stack_tie (gen_rtx_PARALLEL (VOIDmode, p))); diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index b0db8ae508d..fdcf8347812 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -158,6 +158,7 @@ (define_c_enum "unspec" UNSPEC_HASHCHK UNSPEC_XXSPLTIDP_CONST UNSPEC_XXSPLTIW_CONST + UNSPEC_TIE ]) ;; @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block" operands[4] = gen_frame_mem (Pmode, operands[1]); p = rtvec_alloc (1); RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), - const0_rtx); + gen_rtx_UNSPEC (BLKmode, + gen_rtvec (1, const0_rtx), + UNSPEC_TIE)); operands[5] = gen_rtx_PARALLEL (VOIDmode, p); }) @@ -10866,7 +10869,9 @@ (define_expand "restore_stack_nonlocal" operands[5] = gen_frame_mem (Pmode, operands[3]); p = rtvec_alloc (1); RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), - const0_rtx); + gen_rtx_UNSPEC (BLKmode, + gen_rtvec (1, const0_rtx), + UNSPEC_TIE)); operands[6] = gen_rtx_PARALLEL (VOIDmode, p); }) @@ -13898,7 +13903,8 @@ (define_insn "*save_fpregs__r1" ; not be moved over loads from or stores to stack memory. (define_insn "stack_tie" [(match_parallel 0 "tie_operand" - [(set (mem:BLK (reg 1)) (const_int 0))])] + [(set (mem:BLK (reg 1)) + (unspec:BLK [(const_int 0)] UNSPEC_TIE))])] "" "" [(set_attr "length" "0")]) @@ -13910,7 +13916,7 @@ (define_insn "stack_restore_tie" [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r") (plus:SI (match_operand:SI 1 "gpc_reg_operand" "r,r") (match_operand:SI 2 "reg_or_cint_
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi David, David Edelsohn writes: > Hi, Jiufu > > This definitely seems to be a better solution. > > The TARGET_CONST_ANCHOR change should not be part of this patch. Also > there is no ChangeLog for the patch. Thanks a lot for your quick review!! And sorry for the sending this patch in a hurry. I would update the patch accordingly. BR, Jeff (Jiufu Guo) > > This generally looks correct and consistent with other ports. I want > to give Segher a chance to double check it, if he wishes. > > Thanks David > > On Mon, Jun 12, 2023 at 9:19 AM Jiufu Guo wrote: >> >> Hi, >> >> For stack_tie, currently below insn is generated: >> (insn 15 14 16 3 (parallel [ >> (set (mem/c:BLK (reg/f:DI 1 1) [1 A8]) >> (const_int 0 [0])) >> ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie} >> (nil)) >> >> It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])". This maybe >> looks like "a memory block is zerored", while actually stack_tie >> may be more like a placeholder, and does not generate any thing. >> >> To avoid potential misunderstand, "UNPSEC:BLK [(const_int 0)].." could >> be used here like other ports. >> >> This patch does this. Bootstrap®test pass on ppc64{,le}. >> Is this ok for trunk? >> >> BR, >> Jeff (Jiufu Guo) >> >> --- >> gcc/config/rs6000/predicates.md | 11 +++ >> gcc/config/rs6000/rs6000-logue.cc | 4 +++- >> gcc/config/rs6000/rs6000.cc | 4 >> gcc/config/rs6000/rs6000.md | 14 ++ >> 4 files changed, 24 insertions(+), 9 deletions(-) >> >> diff --git a/gcc/config/rs6000/predicates.md >> b/gcc/config/rs6000/predicates.md >> index a16ee30f0c0..4748cb37ce8 100644 >> --- a/gcc/config/rs6000/predicates.md >> +++ b/gcc/config/rs6000/predicates.md >> @@ -1854,10 +1854,13 @@ (define_predicate "stmw_operation" >> (define_predicate "tie_operand" >>(match_code "parallel") >> { >> - return (GET_CODE (XVECEXP (op, 0, 0)) == SET >> - && MEM_P (XEXP (XVECEXP (op, 0, 0), 0)) >> - && GET_MODE (XEXP (XVECEXP (op, 0, 0), 0)) == BLKmode >> - && XEXP (XVECEXP (op, 0, 0), 1) == const0_rtx); >> + rtx set = XVECEXP (op, 0, 0); >> + return (GET_CODE (set) == SET >> + && MEM_P (SET_DEST (set)) >> + && GET_MODE (SET_DEST (set)) == BLKmode >> + && GET_CODE (SET_SRC (set)) == UNSPEC >> + && XINT (SET_SRC (set), 1) == UNSPEC_TIE >> + && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); >> }) >> >> ;; Match a small code model toc reference (or medium and large >> diff --git a/gcc/config/rs6000/rs6000-logue.cc >> b/gcc/config/rs6000/rs6000-logue.cc >> index bc6b153b59f..b99f43a8282 100644 >> --- a/gcc/config/rs6000/rs6000-logue.cc >> +++ b/gcc/config/rs6000/rs6000-logue.cc >> @@ -1463,7 +1463,9 @@ rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed) >>while (--i >= 0) >> { >>rtx mem = gen_frame_mem (BLKmode, regs[i]); >> - RTVEC_ELT (p, i) = gen_rtx_SET (mem, const0_rtx); >> + RTVEC_ELT (p, i) >> + = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, >> const0_rtx), >> + UNSPEC_TIE)); >> } >> >>emit_insn (gen_stack_tie (gen_rtx_PARALLEL (VOIDmode, p))); >> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc >> index d197c3f3289..0c81ebea711 100644 >> --- a/gcc/config/rs6000/rs6000.cc >> +++ b/gcc/config/rs6000/rs6000.cc >> @@ -1760,6 +1760,10 @@ static const struct attribute_spec >> rs6000_attribute_table[] = >> >> #undef TARGET_UPDATE_IPA_FN_TARGET_INFO >> #define TARGET_UPDATE_IPA_FN_TARGET_INFO rs6000_update_ipa_fn_target_info >> + >> +#undef TARGET_CONST_ANCHOR >> +#define TARGET_CONST_ANCHOR 0x8000 >> + >> >> >> /* Processor table. */ >> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md >> index b0db8ae508d..fdcf8347812 100644 >> --- a/gcc/config/rs6000/rs6000.md >> +++ b/gcc/config/rs6000/rs6000.md >> @@ -158,6 +158,7 @@ (define_c_enum "unspec" >> UNSPEC_HASHCHK >> UNSPEC_XXSPLTIDP_CONST >> UNSPEC_XXSPLTIW_CONST >> + UNSPEC_TIE >>]) >> >> ;; >> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block" >>operands[4] = gen_frame_mem (Pmode, operands[1]); >>p = rtvec_alloc (1); >>RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), >> - const0_rtx); >> + gen_rtx_UNSPEC (BLKmode, >> + gen_rtvec (1, const0_rtx), >> + UNSPEC_TIE)); >>operands[5] = gen_rtx_PARALLEL (VOIDmode, p); >> }) >> >> @@ -10866,7 +10869,9 @@ (define_expand "restore_stack_nonlocal" >>operands[5] = gen_frame_mem (Pmode, operands[3]); >>p = rtvec_alloc (1); >>RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), >> - const0_rtx); >> + gen_rtx_UNSPEC (BLKmode, >>
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, Jiufu This definitely seems to be a better solution. The TARGET_CONST_ANCHOR change should not be part of this patch. Also there is no ChangeLog for the patch. This generally looks correct and consistent with other ports. I want to give Segher a chance to double check it, if he wishes. Thanks David On Mon, Jun 12, 2023 at 9:19 AM Jiufu Guo wrote: > > Hi, > > For stack_tie, currently below insn is generated: > (insn 15 14 16 3 (parallel [ > (set (mem/c:BLK (reg/f:DI 1 1) [1 A8]) > (const_int 0 [0])) > ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie} > (nil)) > > It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])". This maybe > looks like "a memory block is zerored", while actually stack_tie > may be more like a placeholder, and does not generate any thing. > > To avoid potential misunderstand, "UNPSEC:BLK [(const_int 0)].." could > be used here like other ports. > > This patch does this. Bootstrap®test pass on ppc64{,le}. > Is this ok for trunk? > > BR, > Jeff (Jiufu Guo) > > --- > gcc/config/rs6000/predicates.md | 11 +++ > gcc/config/rs6000/rs6000-logue.cc | 4 +++- > gcc/config/rs6000/rs6000.cc | 4 > gcc/config/rs6000/rs6000.md | 14 ++ > 4 files changed, 24 insertions(+), 9 deletions(-) > > diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md > index a16ee30f0c0..4748cb37ce8 100644 > --- a/gcc/config/rs6000/predicates.md > +++ b/gcc/config/rs6000/predicates.md > @@ -1854,10 +1854,13 @@ (define_predicate "stmw_operation" > (define_predicate "tie_operand" >(match_code "parallel") > { > - return (GET_CODE (XVECEXP (op, 0, 0)) == SET > - && MEM_P (XEXP (XVECEXP (op, 0, 0), 0)) > - && GET_MODE (XEXP (XVECEXP (op, 0, 0), 0)) == BLKmode > - && XEXP (XVECEXP (op, 0, 0), 1) == const0_rtx); > + rtx set = XVECEXP (op, 0, 0); > + return (GET_CODE (set) == SET > + && MEM_P (SET_DEST (set)) > + && GET_MODE (SET_DEST (set)) == BLKmode > + && GET_CODE (SET_SRC (set)) == UNSPEC > + && XINT (SET_SRC (set), 1) == UNSPEC_TIE > + && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); > }) > > ;; Match a small code model toc reference (or medium and large > diff --git a/gcc/config/rs6000/rs6000-logue.cc > b/gcc/config/rs6000/rs6000-logue.cc > index bc6b153b59f..b99f43a8282 100644 > --- a/gcc/config/rs6000/rs6000-logue.cc > +++ b/gcc/config/rs6000/rs6000-logue.cc > @@ -1463,7 +1463,9 @@ rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed) >while (--i >= 0) > { >rtx mem = gen_frame_mem (BLKmode, regs[i]); > - RTVEC_ELT (p, i) = gen_rtx_SET (mem, const0_rtx); > + RTVEC_ELT (p, i) > + = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, > const0_rtx), > + UNSPEC_TIE)); > } > >emit_insn (gen_stack_tie (gen_rtx_PARALLEL (VOIDmode, p))); > diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc > index d197c3f3289..0c81ebea711 100644 > --- a/gcc/config/rs6000/rs6000.cc > +++ b/gcc/config/rs6000/rs6000.cc > @@ -1760,6 +1760,10 @@ static const struct attribute_spec > rs6000_attribute_table[] = > > #undef TARGET_UPDATE_IPA_FN_TARGET_INFO > #define TARGET_UPDATE_IPA_FN_TARGET_INFO rs6000_update_ipa_fn_target_info > + > +#undef TARGET_CONST_ANCHOR > +#define TARGET_CONST_ANCHOR 0x8000 > + > > > /* Processor table. */ > diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md > index b0db8ae508d..fdcf8347812 100644 > --- a/gcc/config/rs6000/rs6000.md > +++ b/gcc/config/rs6000/rs6000.md > @@ -158,6 +158,7 @@ (define_c_enum "unspec" > UNSPEC_HASHCHK > UNSPEC_XXSPLTIDP_CONST > UNSPEC_XXSPLTIW_CONST > + UNSPEC_TIE >]) > > ;; > @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block" >operands[4] = gen_frame_mem (Pmode, operands[1]); >p = rtvec_alloc (1); >RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), > - const0_rtx); > + gen_rtx_UNSPEC (BLKmode, > + gen_rtvec (1, const0_rtx), > + UNSPEC_TIE)); >operands[5] = gen_rtx_PARALLEL (VOIDmode, p); > }) > > @@ -10866,7 +10869,9 @@ (define_expand "restore_stack_nonlocal" >operands[5] = gen_frame_mem (Pmode, operands[3]); >p = rtvec_alloc (1); >RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), > - const0_rtx); > + gen_rtx_UNSPEC (BLKmode, > + gen_rtvec (1, const0_rtx), > + UNSPEC_TIE)); >operands[6] = gen_rtx_PARALLEL (VOIDmode, p); > }) > > @@ -13898,7 +13903,8 @@ (define_insn "*save_fpregs__r1" > ; not be moved over loads from or stores to stack memory. > (def
[PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, For stack_tie, currently below insn is generated: (insn 15 14 16 3 (parallel [ (set (mem/c:BLK (reg/f:DI 1 1) [1 A8]) (const_int 0 [0])) ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie} (nil)) It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])". This maybe looks like "a memory block is zerored", while actually stack_tie may be more like a placeholder, and does not generate any thing. To avoid potential misunderstand, "UNPSEC:BLK [(const_int 0)].." could be used here like other ports. This patch does this. Bootstrap®test pass on ppc64{,le}. Is this ok for trunk? BR, Jeff (Jiufu Guo) --- gcc/config/rs6000/predicates.md | 11 +++ gcc/config/rs6000/rs6000-logue.cc | 4 +++- gcc/config/rs6000/rs6000.cc | 4 gcc/config/rs6000/rs6000.md | 14 ++ 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md index a16ee30f0c0..4748cb37ce8 100644 --- a/gcc/config/rs6000/predicates.md +++ b/gcc/config/rs6000/predicates.md @@ -1854,10 +1854,13 @@ (define_predicate "stmw_operation" (define_predicate "tie_operand" (match_code "parallel") { - return (GET_CODE (XVECEXP (op, 0, 0)) == SET - && MEM_P (XEXP (XVECEXP (op, 0, 0), 0)) - && GET_MODE (XEXP (XVECEXP (op, 0, 0), 0)) == BLKmode - && XEXP (XVECEXP (op, 0, 0), 1) == const0_rtx); + rtx set = XVECEXP (op, 0, 0); + return (GET_CODE (set) == SET + && MEM_P (SET_DEST (set)) + && GET_MODE (SET_DEST (set)) == BLKmode + && GET_CODE (SET_SRC (set)) == UNSPEC + && XINT (SET_SRC (set), 1) == UNSPEC_TIE + && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); }) ;; Match a small code model toc reference (or medium and large diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc index bc6b153b59f..b99f43a8282 100644 --- a/gcc/config/rs6000/rs6000-logue.cc +++ b/gcc/config/rs6000/rs6000-logue.cc @@ -1463,7 +1463,9 @@ rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed) while (--i >= 0) { rtx mem = gen_frame_mem (BLKmode, regs[i]); - RTVEC_ELT (p, i) = gen_rtx_SET (mem, const0_rtx); + RTVEC_ELT (p, i) + = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), + UNSPEC_TIE)); } emit_insn (gen_stack_tie (gen_rtx_PARALLEL (VOIDmode, p))); diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc index d197c3f3289..0c81ebea711 100644 --- a/gcc/config/rs6000/rs6000.cc +++ b/gcc/config/rs6000/rs6000.cc @@ -1760,6 +1760,10 @@ static const struct attribute_spec rs6000_attribute_table[] = #undef TARGET_UPDATE_IPA_FN_TARGET_INFO #define TARGET_UPDATE_IPA_FN_TARGET_INFO rs6000_update_ipa_fn_target_info + +#undef TARGET_CONST_ANCHOR +#define TARGET_CONST_ANCHOR 0x8000 + /* Processor table. */ diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index b0db8ae508d..fdcf8347812 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -158,6 +158,7 @@ (define_c_enum "unspec" UNSPEC_HASHCHK UNSPEC_XXSPLTIDP_CONST UNSPEC_XXSPLTIW_CONST + UNSPEC_TIE ]) ;; @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block" operands[4] = gen_frame_mem (Pmode, operands[1]); p = rtvec_alloc (1); RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), - const0_rtx); + gen_rtx_UNSPEC (BLKmode, + gen_rtvec (1, const0_rtx), + UNSPEC_TIE)); operands[5] = gen_rtx_PARALLEL (VOIDmode, p); }) @@ -10866,7 +10869,9 @@ (define_expand "restore_stack_nonlocal" operands[5] = gen_frame_mem (Pmode, operands[3]); p = rtvec_alloc (1); RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), - const0_rtx); + gen_rtx_UNSPEC (BLKmode, + gen_rtvec (1, const0_rtx), + UNSPEC_TIE)); operands[6] = gen_rtx_PARALLEL (VOIDmode, p); }) @@ -13898,7 +13903,8 @@ (define_insn "*save_fpregs__r1" ; not be moved over loads from or stores to stack memory. (define_insn "stack_tie" [(match_parallel 0 "tie_operand" - [(set (mem:BLK (reg 1)) (const_int 0))])] + [(set (mem:BLK (reg 1)) + (unspec:BLK [(const_int 0)] UNSPEC_TIE))])] "" "" [(set_attr "length" "0")]) @@ -13910,7 +13916,7 @@ (define_insn "stack_restore_tie" [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r") (plus:SI (match_operand:SI 1 "gpc_reg_operand" "r,r") (match_operand:SI 2 "reg_or_cint_operand" "O,rI"))) - (set (mem:BLK (scratch)) (const_int 0))] + (set (mem:BLK (scratc