Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Mon, Nov 19, 2018 at 12:59:29PM +, Michael Matz wrote:
> Hi,
>
> On Fri, 16 Nov 2018, Segher Boessenkool wrote:
>
> > > I.e. like volatile they can arbitrarily change their value. I don't
> > > know if other peoples mind model is similar, but it certainly is the
> > > model that is implemented in the GIMPLE pipeline (and if memory serves
> > > well of the RTL pipeline as well).
> > >
> > > Copying outof/into pseudos around asms serves no purpose with that
> > > model, it rather makes regvars somewhat "safer" to use, without
> > > actually making them safer (there _is_ nothing safe about them).
> >
> > The only supported case is for inputs and outputs to extended asm.
> > Maybe we should just come up with a syntax so you can specify hard regs
> > for that directly (without needing a separate regclass for every reg).
>
> I would like that, yes. (Of course backward compat would have us support
> the historic usage for some time, but still ...)
Right, or for ten years at least (there are header files that use
register asm!)
I was thinking something like
asm("smth %0,%1" : "=*r0"(x) : "*r1"(y));
where then "*" means the rest of the constraint is just a register name.
I don't know how easy this really is to make work, but do you think this
is a sensible syntax?
Segher
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
Hi, On Fri, 16 Nov 2018, Segher Boessenkool wrote: > > I.e. like volatile they can arbitrarily change their value. I don't > > know if other peoples mind model is similar, but it certainly is the > > model that is implemented in the GIMPLE pipeline (and if memory serves > > well of the RTL pipeline as well). > > > > Copying outof/into pseudos around asms serves no purpose with that > > model, it rather makes regvars somewhat "safer" to use, without > > actually making them safer (there _is_ nothing safe about them). > > The only supported case is for inputs and outputs to extended asm. > Maybe we should just come up with a syntax so you can specify hard regs > for that directly (without needing a separate regclass for every reg). I would like that, yes. (Of course backward compat would have us support the historic usage for some time, but still ...) Ciao, Michael.
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
Hi! On Thu, Nov 15, 2018 at 03:54:03PM +, Michael Matz wrote: > On Wed, 14 Nov 2018, Alexander Monakov wrote: > > On Wed, 14 Nov 2018, Segher Boessenkool wrote: > > > Yeah, using local register vars to access global registers only works > > > by accident. It does work currently though, and people apparently use > > > it, so we shouldn't break it :-/ > > > > In the proposed approach (copying from/to pseudos just before/after the > > asm) we can emulate historic behavior by making uninitialized pseudos > > take values of the corresponding hardregs. If we decide that doing that > > just for must-uninit pseudos is enough, it's a simple extension to the > > existing init-regs pass. Does this sound reasonable? > > Or we can just decide that nothing needs fixing. In particular about this > from Jakub and Segher: > > > > What doesn't work as the reporter expect is assumption that local hard > > > register variables that live across function calls must have their > > > values preserved; they can be modified by the callees. > > > > It would be really nice if we could fix that :-) > > I disagree that there's something to fix. What I would like fixed is the surprising behaviour that people run into time after time again. > My mental model for local reg > vars has always been that such vars are actually an alias for that > register, not comparable to normal automatic variables (so, much like > global reg vars, except that they don't reserve away the register from > regalloc). That was the case long ago, yes. And mostly it still is, but this is not guaranteed behaviour. Things are expanded as being in that reg, but many optimisations can destroy this later. And function calls (some of which are not explicit in the user's program!) will clobber things. And and and. It is very hard to use this feature reliably. > I.e. like volatile they can arbitrarily change their value. > I don't know if other peoples mind model is similar, but it certainly is > the model that is implemented in the GIMPLE pipeline (and if memory serves > well of the RTL pipeline as well). > > Copying outof/into pseudos around asms serves no purpose with that model, > it rather makes regvars somewhat "safer" to use, without actually making > them safer (there _is_ nothing safe about them). The only supported case is for inputs and outputs to extended asm. Maybe we should just come up with a syntax so you can specify hard regs for that directly (without needing a separate regclass for every reg). Segher
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
Hi, On Fri, 16 Nov 2018, Alexander Monakov wrote: > > Not really, it ignores the fact that 'a' can change at any time, which > > is what happens. > > Are you saying that local register variables, in your model, are > essentially unfit for passing operands to inline asm on specific > registers (i.e. their primary purpose)? It seems so, yes. Obviously we _want_ them to be used for this purpose, so something is wrong with my mental model :) I think I'm starting to lean towards your model, hmm. Ciao, Michael.
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Fri, 16 Nov 2018, Michael Matz wrote: > > This follows both your model > > Not really, it ignores the fact that 'a' can change at any time, which is > what happens. Are you saying that local register variables, in your model, are essentially unfit for passing operands to inline asm on specific registers (i.e. their primary purpose)? Alexander
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
Hi,
On Thu, 15 Nov 2018, Alexander Monakov wrote:
> Reading the documentation certainly does not make that impression to me.
> In any case, can you elaborate a bit further please:
>
> 1. Regarding the comparison to 'volatile' qualifier. Suppose you have an
> automatic variable 'int v;' in a correct program. The variable is only used
> for some arithmetic, never passed to asms, does not have its address taken.
I should have been more precise, I meant volatile and mapped to some
device memory. I.e. a register variable is never automatic in that sense.
Every read can return different values even without intervening changes in
source code.
> 2. Are testcases given in PR 87984 valid? Quoting the latest example:
>
> int f(void)
> {
> int o=0, i;
> for (i=0; i<3; i++) {
> register int a asm("eax");
> a = 1;
> asm("add %1, %0" : "+r"(o) : "r"(a));
> asm("xor %%eax, %%eax" ::: "eax");
> }
> return o;
> }
Which is indeed what happens here.
> This follows both your model
Not really, it ignores the fact that 'a' can change at any time, which is
what happens.
> and the documentation to the letter, and yet will return 1 rather than
> 3.
>
> I disagree that it is practical to implement your model on GIMPLE.
I claim that that is what is implemented right now.
Ciao,
Michael.
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On 11/14/2018 02:39 AM, Alexander Monakov wrote: On Tue, 13 Nov 2018, Martin Sebor wrote: In PR 88000 the reporter expects to be able to use an explicit register local variable in a context where it isn't supported i.e., for something other than an input or output of an asm statement: namely to pass it as argument to a user-defined function. GCC emits unexpected object code in this case which the reporter thought was a GCC bug. I appreciate warnings for misuse of extensions in general, but in this particular case I think GCC's behavior is misdesigned, so instead of enshrining a bad engineering choice in a warning and in the manual, I'd rather see GCC implement the extension sanely. Merely changing a normal auto variable to a register asm variable should not invalidate the program. As the manual says, it should amount to providing a hint to the register allocator, at most. Have a look at PR 87984, where code is miscompiled despite following the documentation to the letter. This is because we lower accesses to register variables when transitioning from GIMPLE to RTL incorrectly. Fixing that should make the warning unnecessary. I hope I can work on that before stage 4. I think LLVM is doing the right thing there, and so should we. That would indeed be preferable but it's not something I expect to have the cycles to work on. I put the patch together only because it seemed like an easy way to help keep users from shooting themselves in the foot (to borrow the phrase from the comment in PR 88000). If there's consensus that the general use case of passing hard registers to functions should be supported then I suggest to acknowledge PR 88000 as a bug. We can discuss whether it's possible and worthwhile to warn on the unsupported subset of the use cases. In any case, I think the least we can for now is to clarify in the manual what that supported subset is, since as I (and others) read it, passing hard registers to functions is not. Martin
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On 11/14/2018 02:47 AM, Jakub Jelinek wrote:
On Tue, Nov 13, 2018 at 09:11:41PM -0700, Martin Sebor wrote:
--- gcc/c-family/c-warn.c (revision 266086)
+++ gcc/c-family/c-warn.c (working copy)
@@ -2609,3 +2609,39 @@ warn_for_multistatement_macros (location_t body_lo
inform (guard_loc, "some parts of macro expansion are not guarded by "
"this %qs clause", guard_tinfo_to_string (keyword));
}
+
+
+/* Diagnose unsuported use of explicit hardware register variable ARG
+ as an argument ARGNO to function FNDECL. */
+
+void
+warn_hw_reg_arg (tree fndecl, int argno, tree arg)
+{
+ if (!fndecl)
+return;
+
+ /* Avoid diagnosing GCC intrinsics with no library fallbacks. */
+ if (fndecl_built_in_p (fndecl)
+ && DECL_IS_BUILTIN (fndecl)
+ && !c_decl_implicit (fndecl)
+ && !DECL_ASSEMBLER_NAME_SET_P (fndecl))
+return;
+
+ /* Also avoid diagnosing always_inline functions since those are
+ often used to implement vectorization intrinsics that make use
+ of hardware register variables. */
+ if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fndecl)))
+return;
+
+ /* Diagnose uses of local variables declared asm register. */
+ STRIP_ANY_LOCATION_WRAPPER (arg);
+ if (VAR_P (arg)
+ && !TREE_STATIC (arg)
+ && DECL_HARD_REGISTER (arg)
+ && warning_at (input_location, OPT_Wasm_register_var,
+"unsupported use of hardware register %qD as "
+"argument %d of %qD",
+arg, argno, fndecl))
+inform (DECL_SOURCE_LOCATION (arg),
+ "%qD declared here", arg);
+}
This makes no sense to me. There is nothing unsupported in passing
a local hard register variable to a function, that is well defined,
PR 88000 was resolved as invalid quoting the following sentence
from the manual as the rationale:
The only supported use for this feature is to specify registers
for input and output operands when calling Extended asm.
If these variables are meant to be supported in other contexts
(such as in calls to built-ins or even user-defined functions)
the manual needs to be clarified.
and as your testcase changes show, you broke quite some completely valid
testcases with that.
Let's not be melodramatic. Nothing was "broken" by a proposed
warning. But I did say that "if using explicit register vars in
those functions is meant to be supported despite what the manual
says the patch will need tweaking (as will the manual)."
What doesn't work as the reporter expect is assumption that local hard
register variables that live across function calls must have their values
preserved; they can be modified by the callees.
I'm not sure I understand how what you described as supported
is different from what the test case in 88000 does: i.e., pass
the value of a register variable to a function and expect its
value to be unchanged. If the value happens to be preserved
in some cases/for some registers but not for others, unless
the two sets can be reliably distinguished it seems like a good
candidate for a warning, to help users fall into the same trap.
Users who know what they're doing can easily suppress the warning
and others will fix their code. Is there a way to do distinguish
the two sets of cases?
Martin
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Thu, 15 Nov 2018, Michael Matz wrote:
> I disagree that there's something to fix. My mental model for local reg
> vars has always been that such vars are actually an alias for that
> register, not comparable to normal automatic variables (so, much like
> global reg vars, except that they don't reserve away the register from
> regalloc). I.e. like volatile they can arbitrarily change their value.
> I don't know if other peoples mind model is similar, but it certainly is
> the model that is implemented in the GIMPLE pipeline (and if memory serves
> well of the RTL pipeline as well).
Reading the documentation certainly does not make that impression to me.
In any case, can you elaborate a bit further please:
1. Regarding the comparison to 'volatile' qualifier. Suppose you have an
automatic variable 'int v;' in a correct program. The variable is only used
for some arithmetic, never passed to asms, does not have its address taken.
Thus, changing its
declaration to 'volatile int v;' would not make the program invalid. Now,
can declaring it as 'register int v asm("%rbx");' (with a callee-saved
register) make the program invalid? My reading of the documentation is
that it would provide a regalloc hint and have no ill effects.
2. Are testcases given in PR 87984 valid? Quoting the latest example:
int f(void)
{
int o=0, i;
for (i=0; i<3; i++) {
register int a asm("eax");
a = 1;
asm("add %1, %0" : "+r"(o) : "r"(a));
asm("xor %%eax, %%eax" ::: "eax");
}
return o;
}
This follows both your model and the documentation to the letter, and
yet will return 1 rather than 3.
I disagree that it is practical to implement your model on GIMPLE.
Thanks.
Alexander
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
Hi, On Wed, 14 Nov 2018, Alexander Monakov wrote: > On Wed, 14 Nov 2018, Segher Boessenkool wrote: > > Yeah, using local register vars to access global registers only works > > by accident. It does work currently though, and people apparently use > > it, so we shouldn't break it :-/ > > In the proposed approach (copying from/to pseudos just before/after the > asm) we can emulate historic behavior by making uninitialized pseudos > take values of the corresponding hardregs. If we decide that doing that > just for must-uninit pseudos is enough, it's a simple extension to the > existing init-regs pass. Does this sound reasonable? Or we can just decide that nothing needs fixing. In particular about this from Jakub and Segher: > > What doesn't work as the reporter expect is assumption that local hard > > register variables that live across function calls must have their > > values preserved; they can be modified by the callees. > > It would be really nice if we could fix that :-) I disagree that there's something to fix. My mental model for local reg vars has always been that such vars are actually an alias for that register, not comparable to normal automatic variables (so, much like global reg vars, except that they don't reserve away the register from regalloc). I.e. like volatile they can arbitrarily change their value. I don't know if other peoples mind model is similar, but it certainly is the model that is implemented in the GIMPLE pipeline (and if memory serves well of the RTL pipeline as well). Copying outof/into pseudos around asms serves no purpose with that model, it rather makes regvars somewhat "safer" to use, without actually making them safer (there _is_ nothing safe about them). Ciao, Michael.
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, 14 Nov 2018, Segher Boessenkool wrote: > Yeah, using local register vars to access global registers only works > by accident. It does work currently though, and people apparently use > it, so we shouldn't break it :-/ In the proposed approach (copying from/to pseudos just before/after the asm) we can emulate historic behavior by making uninitialized pseudos take values of the corresponding hardregs. If we decide that doing that just for must-uninit pseudos is enough, it's a simple extension to the existing init-regs pass. Does this sound reasonable? Alexander
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, Nov 14, 2018 at 06:50:38PM +0300, Alexander Monakov wrote: > On Wed, 14 Nov 2018, Segher Boessenkool wrote: > > > I think with "=g" rather than "+g" this example is ok. > > > > No, it needs the register var as an input. That is the whole *point*. > > Hm. I think I see what you meant, but "+g" is not correct either: the > asm, by intent, depends *on the current value in the 'sp' hardreg*, not > *on the current value of some automatic variable that is supposed to be > passed on the 'sp' hardreg to the asm* (which is what expressed by the > input constraint). Yeah, using local register vars to access global registers only works by accident. It does work currently though, and people apparently use it, so we shouldn't break it :-/ Segher
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, 14 Nov 2018, Segher Boessenkool wrote:
> > I think with "=g" rather than "+g" this example is ok.
>
> No, it needs the register var as an input. That is the whole *point*.
Hm. I think I see what you meant, but "+g" is not correct either: the
asm, by intent, depends *on the current value in the 'sp' hardreg*, not
*on the current value of some automatic variable that is supposed to be
passed on the 'sp' hardreg to the asm* (which is what expressed by the
input constraint).
Consider what would happen in the scenario demonstrated in PR 89784:
suppose you have (e.g. after inlining 'retsp' in a loop):
for (int i=0; i<2; i++)
{
register long sp asm ("%rsp");
asm ("" : "+r" (sp));
}
and then after unrolling
register long sp asm ("%rsp");
asm ("" : "+r" (sp));
asm ("" : "+r" (sp));
where only the first asm has an uninitialized input, and the second asm
implies restoring hardreg %rsp to the value in variable sp.
So at a minimum you'd need to use two separate register variables:
register long sp_in asm ("%rsp");
register long sp asm ("%rsp");
asm ("" : "=r" (sp) : "r" (sp_in));
Alexander
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, Nov 14, 2018 at 03:33:43PM +0300, Alexander Monakov wrote:
> On Wed, 14 Nov 2018, Jakub Jelinek wrote:
>
> > On Wed, Nov 14, 2018 at 06:22:51AM -0600, Segher Boessenkool wrote:
> > > Btw, if you just add
> > >
> > > void *
> > > retsp (void)
> > > {
> > > register void *sp __asm ("sp");
> > > asm ("" : "+g" (sp)); // <-- this line
> > > return sp;
> > > }
> > >
> > > everything works fine.
> >
> > Even in what you are proposing, i.e. handle the var as any other var
> > in SSA form and only copy into the hard register right before asm and out of
> > it after it?
> > Because
> > {
> > void *sp;
> > asm ("" : "+g" (sp));
> > return sp;
> > }
> > would store into the register default definition of the SSA_NAME (the var
> > has no initializer).
>
> I think with "=g" rather than "+g" this example is ok.
No, it needs the register var as an input. That is the whole *point*.
It could output elsewhere, like with
void *
retsp (void)
{
register void *sp __asm ("sp");
void *p;
asm ("" : "=g" (p) : "0" (sp));
return p;
}
(which also works reliably with current GCC).
Or like
void *
retsp (void)
{
register void *sp __asm ("sp");
void *p;
asm ("mov %0,%1" : "=r" (p) : "r" (sp));
return p;
}
if you don't want to tie the asm input and output (but use an extra
machine instruction, alas).
Segher
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, Nov 14, 2018 at 01:27:26PM +0100, Jakub Jelinek wrote:
> On Wed, Nov 14, 2018 at 06:22:51AM -0600, Segher Boessenkool wrote:
> > Btw, if you just add
> >
> > void *
> > retsp (void)
> > {
> > register void *sp __asm ("sp");
> > asm ("" : "+g" (sp)); // <-- this line
> > return sp;
> > }
> >
> > everything works fine.
>
> Even in what you are proposing, i.e. handle the var as any other var
> in SSA form and only copy into the hard register right before asm and out of
> it after it?
Yes, *only* in that: with current trunk sp lives in the "sp" hard register
at the "return sp", which cannot work reliably (what value is returned?
It is unspecified).
> Because
> {
> void *sp;
> asm ("" : "+g" (sp));
> return sp;
> }
> would store into the register default definition of the SSA_NAME (the var
> has no initializer).
I'm more concerned about what it looks like in RTL, but sure :-) What
*should* it do before RTL? Not much at all I think, just keep track that
this var is a register asm and that's that?
Segher
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, 14 Nov 2018, Jakub Jelinek wrote:
> On Wed, Nov 14, 2018 at 06:22:51AM -0600, Segher Boessenkool wrote:
> > Btw, if you just add
> >
> > void *
> > retsp (void)
> > {
> > register void *sp __asm ("sp");
> > asm ("" : "+g" (sp)); // <-- this line
> > return sp;
> > }
> >
> > everything works fine.
>
> Even in what you are proposing, i.e. handle the var as any other var
> in SSA form and only copy into the hard register right before asm and out of
> it after it?
> Because
> {
> void *sp;
> asm ("" : "+g" (sp));
> return sp;
> }
> would store into the register default definition of the SSA_NAME (the var
> has no initializer).
I think with "=g" rather than "+g" this example is ok.
Alexander
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, Nov 14, 2018 at 06:22:51AM -0600, Segher Boessenkool wrote:
> Btw, if you just add
>
> void *
> retsp (void)
> {
> register void *sp __asm ("sp");
> asm ("" : "+g" (sp)); // <-- this line
> return sp;
> }
>
> everything works fine.
Even in what you are proposing, i.e. handle the var as any other var
in SSA form and only copy into the hard register right before asm and out of
it after it?
Because
{
void *sp;
asm ("" : "+g" (sp));
return sp;
}
would store into the register default definition of the SSA_NAME (the var
has no initializer).
Jakub
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
Btw, if you just add
void *
retsp (void)
{
register void *sp __asm ("sp");
asm ("" : "+g" (sp)); // <-- this line
return sp;
}
everything works fine.
Segher
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, Nov 14, 2018 at 06:11:37AM -0600, Segher Boessenkool wrote: > It doesn't "break" anything because it currently isn't guaranteed to work > either. There is __builtin_frame_address(0) for this of course (well, > almost the same semantics, and it is actually well-defined and actually > works). Is there user code that tries to do this with a register var? I've seen such code many times over the years, don't have time to perform code searches for such kind of constructs now though. Jakub
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, Nov 14, 2018 at 01:00:43PM +0100, Jakub Jelinek wrote:
> On Wed, Nov 14, 2018 at 05:50:39AM -0600, Segher Boessenkool wrote:
> > > You mean for all local hard register variables living across function
> > > calls
> > > save them into temporary and restore them around the calls?
> > > One issue is that those variables aren't in SSA form, so liveness analysis
> > > is harder. And, would we want to have an exception for the stack pointer?
> > > I mean there is no need for register void *sp __asm ("rsp"); to be
> > > saved/restored that way, it shouldn't change value across function calls.
> > > Plus, as has been mentioned, function calls aren't the only issue here,
> > > e.g. division/modulo etc. that require specific hard registers might
> > > clobber
> > > them too.
> >
> > I was thinking put them in pseudos always, just copy them to the hard reg
> > right before the asm statements that use them (and the other way around,
> > for outputs).
>
> Wouldn't that break all the code that does:
> void *
> retsp (void)
> {
> register void *sp __asm ("sp");
> return sp;
> }
> (or any other (usually fixed) reg, where the user code expects to just
> read the value of that register)?
It doesn't "break" anything because it currently isn't guaranteed to work
either. There is __builtin_frame_address(0) for this of course (well,
almost the same semantics, and it is actually well-defined and actually
works). Is there user code that tries to do this with a register var?
> Storing uninitialized value into it would break the program miserably.
There is no asm statement here.
Segher
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, Nov 14, 2018 at 05:50:39AM -0600, Segher Boessenkool wrote:
> > You mean for all local hard register variables living across function calls
> > save them into temporary and restore them around the calls?
> > One issue is that those variables aren't in SSA form, so liveness analysis
> > is harder. And, would we want to have an exception for the stack pointer?
> > I mean there is no need for register void *sp __asm ("rsp"); to be
> > saved/restored that way, it shouldn't change value across function calls.
> > Plus, as has been mentioned, function calls aren't the only issue here,
> > e.g. division/modulo etc. that require specific hard registers might clobber
> > them too.
>
> I was thinking put them in pseudos always, just copy them to the hard reg
> right before the asm statements that use them (and the other way around,
> for outputs).
Wouldn't that break all the code that does:
void *
retsp (void)
{
register void *sp __asm ("sp");
return sp;
}
(or any other (usually fixed) reg, where the user code expects to just
read the value of that register)?
Storing uninitialized value into it would break the program miserably.
Jakub
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, Nov 14, 2018 at 12:40:01PM +0100, Jakub Jelinek wrote:
> On Wed, Nov 14, 2018 at 05:35:06AM -0600, Segher Boessenkool wrote:
> > On Wed, Nov 14, 2018 at 10:47:57AM +0100, Jakub Jelinek wrote:
> > > This makes no sense to me. There is nothing unsupported in passing
> > > a local hard register variable to a function, that is well defined,
> > > and as your testcase changes show, you broke quite some completely valid
> > > testcases with that.
> >
> > What could perhaps be useful is a warning for a local register var that
> > is not used in any asm?
> >
> > > What doesn't work as the reporter expect is assumption that local hard
> > > register variables that live across function calls must have their values
> > > preserved; they can be modified by the callees.
> >
> > It would be really nice if we could fix that :-)
>
> You mean for all local hard register variables living across function calls
> save them into temporary and restore them around the calls?
> One issue is that those variables aren't in SSA form, so liveness analysis
> is harder. And, would we want to have an exception for the stack pointer?
> I mean there is no need for register void *sp __asm ("rsp"); to be
> saved/restored that way, it shouldn't change value across function calls.
> Plus, as has been mentioned, function calls aren't the only issue here,
> e.g. division/modulo etc. that require specific hard registers might clobber
> them too.
I was thinking put them in pseudos always, just copy them to the hard reg
right before the asm statements that use them (and the other way around,
for outputs).
Segher
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, Nov 14, 2018 at 05:35:06AM -0600, Segher Boessenkool wrote:
> On Wed, Nov 14, 2018 at 10:47:57AM +0100, Jakub Jelinek wrote:
> > This makes no sense to me. There is nothing unsupported in passing
> > a local hard register variable to a function, that is well defined,
> > and as your testcase changes show, you broke quite some completely valid
> > testcases with that.
>
> What could perhaps be useful is a warning for a local register var that
> is not used in any asm?
>
> > What doesn't work as the reporter expect is assumption that local hard
> > register variables that live across function calls must have their values
> > preserved; they can be modified by the callees.
>
> It would be really nice if we could fix that :-)
You mean for all local hard register variables living across function calls
save them into temporary and restore them around the calls?
One issue is that those variables aren't in SSA form, so liveness analysis
is harder. And, would we want to have an exception for the stack pointer?
I mean there is no need for register void *sp __asm ("rsp"); to be
saved/restored that way, it shouldn't change value across function calls.
Plus, as has been mentioned, function calls aren't the only issue here,
e.g. division/modulo etc. that require specific hard registers might clobber
them too.
Jakub
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Wed, Nov 14, 2018 at 10:47:57AM +0100, Jakub Jelinek wrote: > This makes no sense to me. There is nothing unsupported in passing > a local hard register variable to a function, that is well defined, > and as your testcase changes show, you broke quite some completely valid > testcases with that. What could perhaps be useful is a warning for a local register var that is not used in any asm? > What doesn't work as the reporter expect is assumption that local hard > register variables that live across function calls must have their values > preserved; they can be modified by the callees. It would be really nice if we could fix that :-) Segher
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Tue, Nov 13, 2018 at 09:11:41PM -0700, Martin Sebor wrote:
> --- gcc/c-family/c-warn.c (revision 266086)
> +++ gcc/c-family/c-warn.c (working copy)
> @@ -2609,3 +2609,39 @@ warn_for_multistatement_macros (location_t body_lo
> inform (guard_loc, "some parts of macro expansion are not guarded by "
> "this %qs clause", guard_tinfo_to_string (keyword));
> }
> +
> +
> +/* Diagnose unsuported use of explicit hardware register variable ARG
> + as an argument ARGNO to function FNDECL. */
> +
> +void
> +warn_hw_reg_arg (tree fndecl, int argno, tree arg)
> +{
> + if (!fndecl)
> +return;
> +
> + /* Avoid diagnosing GCC intrinsics with no library fallbacks. */
> + if (fndecl_built_in_p (fndecl)
> + && DECL_IS_BUILTIN (fndecl)
> + && !c_decl_implicit (fndecl)
> + && !DECL_ASSEMBLER_NAME_SET_P (fndecl))
> +return;
> +
> + /* Also avoid diagnosing always_inline functions since those are
> + often used to implement vectorization intrinsics that make use
> + of hardware register variables. */
> + if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fndecl)))
> +return;
> +
> + /* Diagnose uses of local variables declared asm register. */
> + STRIP_ANY_LOCATION_WRAPPER (arg);
> + if (VAR_P (arg)
> + && !TREE_STATIC (arg)
> + && DECL_HARD_REGISTER (arg)
> + && warning_at (input_location, OPT_Wasm_register_var,
> + "unsupported use of hardware register %qD as "
> + "argument %d of %qD",
> + arg, argno, fndecl))
> +inform (DECL_SOURCE_LOCATION (arg),
> + "%qD declared here", arg);
> +}
This makes no sense to me. There is nothing unsupported in passing
a local hard register variable to a function, that is well defined,
and as your testcase changes show, you broke quite some completely valid
testcases with that.
What doesn't work as the reporter expect is assumption that local hard
register variables that live across function calls must have their values
preserved; they can be modified by the callees.
Jakub
Re: [PATCH] diagnose unsupported uses of hardware register variables (PR 88000)
On Tue, 13 Nov 2018, Martin Sebor wrote: > In PR 88000 the reporter expects to be able to use an explicit > register local variable in a context where it isn't supported > i.e., for something other than an input or output of an asm > statement: namely to pass it as argument to a user-defined > function. GCC emits unexpected object code in this case which > the reporter thought was a GCC bug. I appreciate warnings for misuse of extensions in general, but in this particular case I think GCC's behavior is misdesigned, so instead of enshrining a bad engineering choice in a warning and in the manual, I'd rather see GCC implement the extension sanely. Merely changing a normal auto variable to a register asm variable should not invalidate the program. As the manual says, it should amount to providing a hint to the register allocator, at most. Have a look at PR 87984, where code is miscompiled despite following the documentation to the letter. This is because we lower accesses to register variables when transitioning from GIMPLE to RTL incorrectly. Fixing that should make the warning unnecessary. I hope I can work on that before stage 4. I think LLVM is doing the right thing there, and so should we. Thanks. Alexander
