Re: typeof and operands in named address spaces
On Thu, Nov 5, 2020 at 8:26 AM Richard Biener
wrote:
>
> On Wed, Nov 4, 2020 at 7:33 PM Uros Bizjak via Gcc wrote:
> >
> > Hello!
> >
> > I was looking at the recent linux patch series [1] where segment
> > qualifiers (named address spaces) were introduced to handle percpu
> > variables. In the patch [2], the author mentions that:
> >
> > --q--
> > Unfortunately, gcc does not provide a way to remove segment
> > qualifiers, which is needed to use typeof() to create local instances
> > of the per-cpu variable. For this reason, do not use the segment
> > qualifier for per-cpu variables, and do casting using the segment
> > qualifier instead.
> > --/q--
> >
> > The core of the problem can be seen with the following testcase:
> >
> > --cut here--
> > #define foo(_var)\
> > ({\
> > typeof(_var) tmp__;\
>
> Looks like writing
>
> typeof((typeof(_var))0) tmp__;
>
> makes it work. Assumes there's a literal zero for the type of course.
This is very limiting assumption, which already breaks for the following test:
--cut here--
typedef struct { short a; short b; } pair_t;
#define foo(_var) \
({ \
typeof((typeof(_var))0) tmp__; \
asm ("mov %1, %0" : "=r"(tmp__) : "m"(_var));\
tmp__; \
})
__seg_fs pair_t x;
pair_t
test (void)
{
pair_t y;
y = foo (x);
return y;
}
--cut here--
So, what about introducing e.g. typeof_noas (not sure about the name)
that would simply strip the address space from typeof?
> Basically I try to get at a rvalue for the typeof.
>
> Is there a way to query the address space of an object so I can
> put another variable in the same address space?
I think that would go hand in hand with the above typeof_noas. Perhaps
typeof_as, that would return the address space of the variable?
> > asm ("mov %1, %0" : "=r"(tmp__) : "m"(_var));\
> > tmp__;\
> > })
> >
> > __seg_fs int x;
> >
> > int test (void)
> > {
> > int y;
> >
> > y = foo (x);
> > return y;
> > }
> > --cut here--
> >
> > when compiled with -O2 for x86 target, the compiler reports:
> >
> > pcpu.c: In function ‘test’:
> > pcpu.c:14:3: error: ‘__seg_fs’ specified for auto variable ‘tmp__’
> >
> > It looks to me that the compiler should remove address space
> > information when typeof is used, otherwise, there is no way to use
> > typeof as intended in the above example.
> >
> > A related problem is exposed when we want to cast address from the
> > named address space to a generic address space (e.g. to use it with
> > LEA):
> >
> > --cut here--
> > typedef __UINTPTR_TYPE__ uintptr_t;
> >
> > __seg_fs int x;
> >
> > uintptr_t test (void)
> > {
> > uintptr_t *p = (uintptr_t *) &y;
>
>uintptr_t *p = (uintptr_t *)(uintptr_t) &y;
Indeed, this works as expected.
> works around the warning. I think the wording you cite
> suggests (uintptr_t) &y here, not sure if there's a reliable
> way to get the lea with just a uintptr_t operand though.
No, because we have to use the "m" constraint for the LEA. We get the
following error:
as1.c:10:49: error: memory input 1 is not directly addressable
Uros.
Re: typeof and operands in named address spaces
On Thu, 5 Nov 2020, Uros Bizjak via Gcc wrote: > > Looks like writing > > > > typeof((typeof(_var))0) tmp__; > > > > makes it work. Assumes there's a literal zero for the type of course. > > This is very limiting assumption, which already breaks for the following test: To elaborate Richard's idea, you need a way to decay lvalue to rvalue inside the typeof to strip the address space; if you need the macro to work for more types than just scalar types, the following expression may be useful: typeof(0?(_var):(_var)) (though there's a bug: +(_var) should also suffice for scalar types, but somehow GCC keeps the address space on the resulting rvalue) But I wonder if you actually need this at all: > > works around the warning. I think the wording you cite > > suggests (uintptr_t) &y here, not sure if there's a reliable > > way to get the lea with just a uintptr_t operand though. > > No, because we have to use the "m" constraint for the LEA. We get the > following error: What is the usecase for stripping the address space for asm operands? >From reading the patch I understand the kernel wants to pass qualified lvalues to inline assembly to get lea , %fs: LEA without the %fs will produce the offset within the segment, which you can obtain simply by casting the pointer to intptr_t in the first place. Alexander
Re: typeof and operands in named address spaces
On Thu, Nov 5, 2020 at 9:56 AM Uros Bizjak wrote:
>
> On Thu, Nov 5, 2020 at 8:26 AM Richard Biener
> wrote:
> >
> > On Wed, Nov 4, 2020 at 7:33 PM Uros Bizjak via Gcc wrote:
> > >
> > > Hello!
> > >
> > > I was looking at the recent linux patch series [1] where segment
> > > qualifiers (named address spaces) were introduced to handle percpu
> > > variables. In the patch [2], the author mentions that:
> > >
> > > --q--
> > > Unfortunately, gcc does not provide a way to remove segment
> > > qualifiers, which is needed to use typeof() to create local instances
> > > of the per-cpu variable. For this reason, do not use the segment
> > > qualifier for per-cpu variables, and do casting using the segment
> > > qualifier instead.
> > > --/q--
> > >
> > > The core of the problem can be seen with the following testcase:
> > >
> > > --cut here--
> > > #define foo(_var)\
> > > ({\
> > > typeof(_var) tmp__;\
> >
> > Looks like writing
> >
> > typeof((typeof(_var))0) tmp__;
> >
> > makes it work. Assumes there's a literal zero for the type of course.
>
> This is very limiting assumption, which already breaks for the following test:
>
> --cut here--
> typedef struct { short a; short b; } pair_t;
>
> #define foo(_var) \
> ({ \
> typeof((typeof(_var))0) tmp__; \
> asm ("mov %1, %0" : "=r"(tmp__) : "m"(_var));\
> tmp__; \
> })
>
> __seg_fs pair_t x;
>
> pair_t
> test (void)
> {
> pair_t y;
>
> y = foo (x);
> return y;
> }
> --cut here--
>
> So, what about introducing e.g. typeof_noas (not sure about the name)
> that would simply strip the address space from typeof?
Well, I think we should fix typeof to not retain the address space. It's
probably our implementation detail of having those in TYPE_QUALS
that exposes the issue and not standard mandated.
The rvalue trick is to avoid depending on a "fixed" GCC.
Joseph should know how typeof should behave here.
Richard.
> > Basically I try to get at a rvalue for the typeof.
> >
> > Is there a way to query the address space of an object so I can
> > put another variable in the same address space?
>
> I think that would go hand in hand with the above typeof_noas. Perhaps
> typeof_as, that would return the address space of the variable?
>
> > > asm ("mov %1, %0" : "=r"(tmp__) : "m"(_var));\
> > > tmp__;\
> > > })
> > >
> > > __seg_fs int x;
> > >
> > > int test (void)
> > > {
> > > int y;
> > >
> > > y = foo (x);
> > > return y;
> > > }
> > > --cut here--
> > >
> > > when compiled with -O2 for x86 target, the compiler reports:
> > >
> > > pcpu.c: In function ‘test’:
> > > pcpu.c:14:3: error: ‘__seg_fs’ specified for auto variable ‘tmp__’
> > >
> > > It looks to me that the compiler should remove address space
> > > information when typeof is used, otherwise, there is no way to use
> > > typeof as intended in the above example.
> > >
> > > A related problem is exposed when we want to cast address from the
> > > named address space to a generic address space (e.g. to use it with
> > > LEA):
> > >
> > > --cut here--
> > > typedef __UINTPTR_TYPE__ uintptr_t;
> > >
> > > __seg_fs int x;
> > >
> > > uintptr_t test (void)
> > > {
> > > uintptr_t *p = (uintptr_t *) &y;
> >
> >uintptr_t *p = (uintptr_t *)(uintptr_t) &y;
>
> Indeed, this works as expected.
>
> > works around the warning. I think the wording you cite
> > suggests (uintptr_t) &y here, not sure if there's a reliable
> > way to get the lea with just a uintptr_t operand though.
>
> No, because we have to use the "m" constraint for the LEA. We get the
> following error:
>
> as1.c:10:49: error: memory input 1 is not directly addressable
>
> Uros.
Re: typeof and operands in named address spaces
On Thu, Nov 05, 2020 at 10:45:59AM +0100, Richard Biener wrote: > Well, I think we should fix typeof to not retain the address space. It's > probably our implementation detail of having those in TYPE_QUALS > that exposes the issue and not standard mandated. > > The rvalue trick is to avoid depending on a "fixed" GCC. > > Joseph should know how typeof should behave here. For other qualifiers like const it has been discussed recently in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97702 and I think the address space qualifiers should work the same as others. Jakub
Re: typeof and operands in named address spaces
On Thu, Nov 5, 2020 at 10:36 AM Alexander Monakov wrote: > > On Thu, 5 Nov 2020, Uros Bizjak via Gcc wrote: > > > > Looks like writing > > > > > > typeof((typeof(_var))0) tmp__; > > > > > > makes it work. Assumes there's a literal zero for the type of course. > > > > This is very limiting assumption, which already breaks for the following > > test: > > To elaborate Richard's idea, you need a way to decay lvalue to rvalue inside > the typeof to strip the address space; if you need the macro to work for > more types than just scalar types, the following expression may be useful: > > typeof(0?(_var):(_var)) > > (though there's a bug: +(_var) should also suffice for scalar types, but > somehow GCC keeps the address space on the resulting rvalue) > > But I wonder if you actually need this at all: > > > > works around the warning. I think the wording you cite > > > suggests (uintptr_t) &y here, not sure if there's a reliable > > > way to get the lea with just a uintptr_t operand though. > > > > No, because we have to use the "m" constraint for the LEA. We get the > > following error: > > What is the usecase for stripping the address space for asm operands? Please see the end of [2], where the offset to is passed in %rsi to the call to this_cpu_cmpxchg16b_emu. this_cpu_cmpxchg16b_emu implements access with PER_CPU_VAR((%rsi)), which expands to %gs:(%rsi), so it is the same as %gs: in cmpxchg16b alternative. The offset is loaded by lea , %rsi to %rsi reg. > From reading the patch I understand the kernel wants to pass qualified > lvalues to inline assembly to get > > lea , %fs: No, this will emit an assembler warning that "segment override on 'lea' is ineffectual". Uros. > LEA without the %fs will produce the offset within the segment, which > you can obtain simply by casting the pointer to intptr_t in the first place. > Alexander
Re: typeof and operands in named address spaces
On Thu, Nov 5, 2020 at 10:36 AM Alexander Monakov wrote:
>
> On Thu, 5 Nov 2020, Uros Bizjak via Gcc wrote:
>
> > > Looks like writing
> > >
> > > typeof((typeof(_var))0) tmp__;
> > >
> > > makes it work. Assumes there's a literal zero for the type of course.
> >
> > This is very limiting assumption, which already breaks for the following
> > test:
>
> To elaborate Richard's idea, you need a way to decay lvalue to rvalue inside
> the typeof to strip the address space; if you need the macro to work for
> more types than just scalar types, the following expression may be useful:
>
> typeof(0?(_var):(_var))
Great, this works well for various operand types.
> (though there's a bug: +(_var) should also suffice for scalar types, but
> somehow GCC keeps the address space on the resulting rvalue)
>
> But I wonder if you actually need this at all:
The posted example is a bit naive, because assignment and basic
operations can be implemented directly, e.g.:
__seg_fs int x;
void
test (void)
{
x &= 1;
}
compiles to:
andl$1, %fs:x(%rip)
without any macro usage at all. However, several operations, such as
xadd and cmpxchg are implemented using assembly templates (see e.g.
arch/x86/include/asm/percpu.h), where local instances are needed.
Uros.
Re: typeof and operands in named address spaces
On Thu, 5 Nov 2020, Uros Bizjak via Gcc wrote:
> > What is the usecase for stripping the address space for asm operands?
>
> Please see the end of [2], where the offset to is passed in %rsi
> to the call to this_cpu_cmpxchg16b_emu. this_cpu_cmpxchg16b_emu
> implements access with PER_CPU_VAR((%rsi)), which expands to
> %gs:(%rsi), so it is the same as %gs: in cmpxchg16b alternative.
> The offset is loaded by lea , %rsi to %rsi reg.
I see, thanks. But then with the typeof-stripping-address-space solution
you'd be making a very evil cast (producing address of an object that
does not actually exist in the generic address space). I can write such
a solution, but it is clearly Undefined Behavior:
#define strip_as(mem) (*(__typeof(0?(mem):(mem))*)(intptr_t)&(mem))
void foo(__seg_fs int *x)
{
asm("# %0" :: "m"(x[1]));
asm("# %0" :: "m"(strip_as(x[1])));
}
yields
foo:
# %fs:4(%rdi)
# 4(%rdi)
ret
I think a clean future solution is adding a operand modifier that would
print the memory operand without the segment prefix.
Alexander
Re: typeof and operands in named address spaces
On Thu, Nov 5, 2020 at 12:38 PM Alexander Monakov wrote:
>
> On Thu, 5 Nov 2020, Uros Bizjak via Gcc wrote:
>
> > > What is the usecase for stripping the address space for asm operands?
> >
> > Please see the end of [2], where the offset to is passed in %rsi
> > to the call to this_cpu_cmpxchg16b_emu. this_cpu_cmpxchg16b_emu
> > implements access with PER_CPU_VAR((%rsi)), which expands to
> > %gs:(%rsi), so it is the same as %gs: in cmpxchg16b alternative.
> > The offset is loaded by lea , %rsi to %rsi reg.
>
> I see, thanks. But then with the typeof-stripping-address-space solution
> you'd be making a very evil cast (producing address of an object that
> does not actually exist in the generic address space). I can write such
> a solution, but it is clearly Undefined Behavior:
>
> #define strip_as(mem) (*(__typeof(0?(mem):(mem))*)(intptr_t)&(mem))
>
> void foo(__seg_fs int *x)
> {
> asm("# %0" :: "m"(x[1]));
> asm("# %0" :: "m"(strip_as(x[1])));
> }
>
> yields
>
> foo:
> # %fs:4(%rdi)
> # 4(%rdi)
> ret
>
>
> I think a clean future solution is adding a operand modifier that would
> print the memory operand without the segment prefix.
I was also thinking of introducing of operand modifier, but Richi
advises the following:
--cut here--
typedef __UINTPTR_TYPE__ uintptr_t;
__seg_fs int x;
uintptr_t test (void)
{
uintptr_t *p = (uintptr_t *)(uintptr_t) &x;
uintptr_t addr;
asm volatile ("lea %1, %0" : "=r"(addr) : "m"(*p));
return addr;
}
--cut here--
Please note that the gcc documentation says:
--q--
6.17.4 x86 Named Address Spaces
---
On the x86 target, variables may be declared as being relative to the
'%fs' or '%gs' segments.
'__seg_fs'
'__seg_gs'
The object is accessed with the respective segment override prefix.
The respective segment base must be set via some method specific to
the operating system. Rather than require an expensive system call
to retrieve the segment base, these address spaces are not
considered to be subspaces of the generic (flat) address space.
This means that explicit casts are required to convert pointers
between these address spaces and the generic address space. In
practice the application should cast to 'uintptr_t' and apply the
segment base offset that it installed previously.
The preprocessor symbols '__SEG_FS' and '__SEG_GS' are defined when
these address spaces are supported.
--/q--
Uros.
Re: typeof and operands in named address spaces
On Thu, 5 Nov 2020, Uros Bizjak wrote:
> On Thu, Nov 5, 2020 at 12:38 PM Alexander Monakov wrote:
> >
> > On Thu, 5 Nov 2020, Uros Bizjak via Gcc wrote:
> >
> > > > What is the usecase for stripping the address space for asm operands?
> > >
> > > Please see the end of [2], where the offset to is passed in %rsi
> > > to the call to this_cpu_cmpxchg16b_emu. this_cpu_cmpxchg16b_emu
> > > implements access with PER_CPU_VAR((%rsi)), which expands to
> > > %gs:(%rsi), so it is the same as %gs: in cmpxchg16b alternative.
> > > The offset is loaded by lea , %rsi to %rsi reg.
> >
> > I see, thanks. But then with the typeof-stripping-address-space solution
> > you'd be making a very evil cast (producing address of an object that
> > does not actually exist in the generic address space). I can write such
> > a solution, but it is clearly Undefined Behavior:
> >
> > #define strip_as(mem) (*(__typeof(0?(mem):(mem))*)(intptr_t)&(mem))
> >
> > void foo(__seg_fs int *x)
> > {
> > asm("# %0" :: "m"(x[1]));
> > asm("# %0" :: "m"(strip_as(x[1])));
> > }
> >
> > yields
> >
> > foo:
> > # %fs:4(%rdi)
> > # 4(%rdi)
> > ret
> >
> >
> > I think a clean future solution is adding a operand modifier that would
> > print the memory operand without the segment prefix.
>
> I was also thinking of introducing of operand modifier, but Richi
> advises the following:
>
> --cut here--
> typedef __UINTPTR_TYPE__ uintptr_t;
>
> __seg_fs int x;
>
> uintptr_t test (void)
> {
> uintptr_t *p = (uintptr_t *)(uintptr_t) &x;
> uintptr_t addr;
>
> asm volatile ("lea %1, %0" : "=r"(addr) : "m"(*p));
>
> return addr;
> }
This is even worse undefined behavior compared to my solution above:
this code references memory in uintptr_t type, while mine preserves the
original type via __typeof. So this can visibly break with TBAA (though
the kernel uses -fno-strict-aliasing, so this particular concern wouldn't
apply there).
If you don't care about preserving sizeof and type you can use a cast to char:
#define strip_as(mem) (*(char *)(intptr_t)&(mem))
Alexander
Re: typeof and operands in named address spaces
On Thu, Nov 5, 2020 at 1:16 PM Alexander Monakov via Gcc
wrote:
>
>
>
> On Thu, 5 Nov 2020, Uros Bizjak wrote:
>
> > On Thu, Nov 5, 2020 at 12:38 PM Alexander Monakov
> > wrote:
> > >
> > > On Thu, 5 Nov 2020, Uros Bizjak via Gcc wrote:
> > >
> > > > > What is the usecase for stripping the address space for asm operands?
> > > >
> > > > Please see the end of [2], where the offset to is passed in %rsi
> > > > to the call to this_cpu_cmpxchg16b_emu. this_cpu_cmpxchg16b_emu
> > > > implements access with PER_CPU_VAR((%rsi)), which expands to
> > > > %gs:(%rsi), so it is the same as %gs: in cmpxchg16b alternative.
> > > > The offset is loaded by lea , %rsi to %rsi reg.
> > >
> > > I see, thanks. But then with the typeof-stripping-address-space solution
> > > you'd be making a very evil cast (producing address of an object that
> > > does not actually exist in the generic address space). I can write such
> > > a solution, but it is clearly Undefined Behavior:
> > >
> > > #define strip_as(mem) (*(__typeof(0?(mem):(mem))*)(intptr_t)&(mem))
> > >
> > > void foo(__seg_fs int *x)
> > > {
> > > asm("# %0" :: "m"(x[1]));
> > > asm("# %0" :: "m"(strip_as(x[1])));
> > > }
> > >
> > > yields
> > >
> > > foo:
> > > # %fs:4(%rdi)
> > > # 4(%rdi)
> > > ret
> > >
> > >
> > > I think a clean future solution is adding a operand modifier that would
> > > print the memory operand without the segment prefix.
> >
> > I was also thinking of introducing of operand modifier, but Richi
> > advises the following:
> >
> > --cut here--
> > typedef __UINTPTR_TYPE__ uintptr_t;
> >
> > __seg_fs int x;
> >
> > uintptr_t test (void)
> > {
> > uintptr_t *p = (uintptr_t *)(uintptr_t) &x;
> > uintptr_t addr;
> >
> > asm volatile ("lea %1, %0" : "=r"(addr) : "m"(*p));
> >
> > return addr;
> > }
>
> This is even worse undefined behavior compared to my solution above:
> this code references memory in uintptr_t type, while mine preserves the
> original type via __typeof. So this can visibly break with TBAA (though
> the kernel uses -fno-strict-aliasing, so this particular concern wouldn't
> apply there).
>
> If you don't care about preserving sizeof and type you can use a cast to char:
>
> #define strip_as(mem) (*(char *)(intptr_t)&(mem))
But in the end, on x86 the (uintptr_t)&x cast yields you exactly
the offset from the segment register, no? The casting back
to (uintrptr_t *) and the "dereference" is just because the
inline asm is not able to build the lea otherwise? that said,
sth like
asm volatile ("lea fs:%1, %0" : "=r"(addr) : "r" ((uintptr_t)&x));
with the proper asm template should likely be used.
Of course in case the kernel wants transparent handling of
non-fs and fs-based cases that will be off.
Richard.
>
> Alexander
Re: typeof and operands in named address spaces
On Thu, Nov 5, 2020 at 1:14 PM Alexander Monakov wrote:
> > I was also thinking of introducing of operand modifier, but Richi
> > advises the following:
> >
> > --cut here--
> > typedef __UINTPTR_TYPE__ uintptr_t;
> >
> > __seg_fs int x;
> >
> > uintptr_t test (void)
> > {
> > uintptr_t *p = (uintptr_t *)(uintptr_t) &x;
> > uintptr_t addr;
> >
> > asm volatile ("lea %1, %0" : "=r"(addr) : "m"(*p));
> >
> > return addr;
> > }
>
> This is even worse undefined behavior compared to my solution above:
> this code references memory in uintptr_t type, while mine preserves the
> original type via __typeof. So this can visibly break with TBAA (though
> the kernel uses -fno-strict-aliasing, so this particular concern wouldn't
> apply there).
Agreed, but I was trying to solve this lone use case in the kernel. It
fits this particular usage, so I found a bit of overkill to implement
the otherwise useless operand modifier in gcc. As discussed
previously, these hacks are needed exclusively in asm templates, they
are not needed in "normal" C code.
>
> If you don't care about preserving sizeof and type you can use a cast to char:
>
> #define strip_as(mem) (*(char *)(intptr_t)&(mem))
I hope that a developer from kernel can chime in and express their
opinion on the proposed approaches.
Uros.
Re: typeof and operands in named address spaces
On Thu, Nov 5, 2020 at 1:24 PM Richard Biener
wrote:
> > This is even worse undefined behavior compared to my solution above:
> > this code references memory in uintptr_t type, while mine preserves the
> > original type via __typeof. So this can visibly break with TBAA (though
> > the kernel uses -fno-strict-aliasing, so this particular concern wouldn't
> > apply there).
> >
> > If you don't care about preserving sizeof and type you can use a cast to
> > char:
> >
> > #define strip_as(mem) (*(char *)(intptr_t)&(mem))
>
> But in the end, on x86 the (uintptr_t)&x cast yields you exactly
> the offset from the segment register, no? The casting back
> to (uintrptr_t *) and the "dereference" is just because the
> inline asm is not able to build the lea otherwise? that said,
> sth like
>
> asm volatile ("lea fs:%1, %0" : "=r"(addr) : "r" ((uintptr_t)&x));
No, this is not how LEA operates. It needs a memory input operand. The
above will report "operand type mismatch for 'lea'" error.
Uros.
Re: typeof and operands in named address spaces
On Thu, Nov 5, 2020 at 1:32 PM Uros Bizjak wrote:
>
> On Thu, Nov 5, 2020 at 1:24 PM Richard Biener
> wrote:
>
> > > This is even worse undefined behavior compared to my solution above:
> > > this code references memory in uintptr_t type, while mine preserves the
> > > original type via __typeof. So this can visibly break with TBAA (though
> > > the kernel uses -fno-strict-aliasing, so this particular concern wouldn't
> > > apply there).
> > >
> > > If you don't care about preserving sizeof and type you can use a cast to
> > > char:
> > >
> > > #define strip_as(mem) (*(char *)(intptr_t)&(mem))
> >
> > But in the end, on x86 the (uintptr_t)&x cast yields you exactly
> > the offset from the segment register, no? The casting back
> > to (uintrptr_t *) and the "dereference" is just because the
> > inline asm is not able to build the lea otherwise? that said,
> > sth like
> >
> > asm volatile ("lea fs:%1, %0" : "=r"(addr) : "r" ((uintptr_t)&x));
>
> No, this is not how LEA operates. It needs a memory input operand. The
> above will report "operand type mismatch for 'lea'" error.
The following will work:
asm volatile ("lea (%1), %0" : "=r"(addr) : "r"((uintptr_t)&x));
Uros.
Re: Dead Field Elimination and Field Reordering
On Tue, Nov 3, 2020 at 5:21 PM Erick Ochoa wrote: > > Thanks for the review Richard I'll address what I can. I also provide > maybe some hindsight into some of the design decisions here. I'm not > trying to be defensive just hoping to illuminate why some decisions were > made and how some criticisms may fail to really address the reason why > these designs were made. > > On 03/11/2020 15:58, Richard Biener wrote: > > On Fri, Oct 30, 2020 at 6:44 PM Erick Ochoa > > wrote: > >> > >> Hello again, > >> > >> I've been working on several implementations of data layout > >> optimizations for GCC, and I am again kindly requesting for a review of > >> the type escape based dead field elimination and field reorg. > >> > >> Thanks to everyone that has helped me. The main differences between the > >> previous commits have been fixing the style, adding comments explaining > >> classes and families of functions, exit gracefully if we handle unknown > >> gimple syntax, and added a heuristic to handle void* casts. > >> > >> This patchset is organized in the following way: > >> > >> * Adds a link-time warning if dead fields are detected > >> * Allows for the dead-field elimination transformation to be applied > >> * Reorganizes fields in structures. > >> * Adds some documentation > >> * Gracefully does not apply transformation if unknown syntax is detected. > >> * Adds a heuristic to handle void* casts > >> > >> I have tested this transformations as extensively as I can. The way to > >> trigger these transformations are: > >> > >> -fipa-field-reorder and -fipa-type-escape-analysis > >> > >> Having said that, I welcome all criticisms and will try to address those > >> criticisms which I can. Please let me know if you have any questions or > >> comments, I will try to answer in a timely manner. > >> > >> The code is in: > >> > >> refs/vendors/ARM/heads/arm-struct-reorg-wip > >> > >> Future work includes extending the current heuristic with ipa-modref > >> extending the analysis to use IPA-PTA as discussed previously. > >> > >> Few notes: > >> > >> * Currently it is not safe to use -fipa-sra. > >> * I added some tests which are now failing by default. This is because > >> there is no way to safely determine within the test case that a layout > >> has been transformed. I used to determine that a field was eliminated > >> doing pointer arithmetic on the fields. And since that is not safe, the > >> analysis decides not to apply the transformation. There is a way to deal > >> with this (add a flag to allow the address of a field to be taken) but I > >> wanted to hear other possibilities to see if there is a better option. > >> * At this point we’d like to thank the again GCC community for their > >> patient help so far on the mailing list and in other channels. And we > >> ask for your support in terms of feedback, comments and testing. > > > > I've only had a brief look at the branch - if you want to even have a > > remote chance of making this stage1 you should break the branch > > up into a proper patch series and post it with appropriate ChangeLogs > > and descriptions. > > > > First, standard includes may _not_ be included after including system.h, > > in fact, they _need_ to be included from system.h - that includes > > things like or . There are "convenient" defines you > > can use like > > > > #define INCLUDE_SET > > #include "system.h" > > > > and system.h will do what you want. Not to say that you should > > use GCCs containers and not the standard library ones. > > Thanks I didn't know this! > > > > > You expose way too many user-visible command-line options. > > Yes, I can certainly remove some debugging flags. > > > > > All the stmt / tree walking "meta" code should be avoided - it > > would need to be touched each time we change GIMPLE or > > GENERIC. Instead use available walkers if you really need > > it in such DFS-ish way. > > There are two points being made here: > 1. Use the available walkers > 2. Changing gimple would imply changes to your code > > First: > > I did tried using the available walkers in the past, and the walk_tree > function does not contain a post-order callback. We really need to > propagate information from the gimple leaf nodes back to the root, and > as a way to achieve this (and probably other things like maintaining a > stack of the nodes visited to reach the current node) we really need a > post-order callback. > > We did have a conversation about this where you pointed out this pattern: > > *walk_subtrees = 0; > walk_tree (.. interesting subexpressions ... ); > do post-order work > > In the preorder hook, but this only works with simple expressions and we > need more complicated machinery. > > Furthermore, I did look into extending the current walk_tree function > with post-order callbacks but due to implementation details in the > function (tail-recursion), we both agreed that having both > tail-recursion AND a post-order hook was impossible. Yes, I remember the discu
Re: typeof and operands in named address spaces
On Thu, 5 Nov 2020, Uros Bizjak via Gcc wrote:
> > No, this is not how LEA operates. It needs a memory input operand. The
> > above will report "operand type mismatch for 'lea'" error.
>
> The following will work:
>
> asm volatile ("lea (%1), %0" : "=r"(addr) : "r"((uintptr_t)&x));
This is the same as a plain move though, and the cast to uintptr_t doesn't
do anything, you can simply pass "r"(&x) to the same effect.
The main advantage of passing a "fake" memory location for use with lea is
avoiding base+offset computation outside the asm. If you're okay with one
extra register tied up by the asm, just pass the address to the asm directly:
void foo(__seg_fs int *x)
{
asm("# %0 (%1)" :: "m"(x[1]), "r"(&x[1]));
asm("# %0 (%1)" :: "m"(x[0]), "r"(&x[0]));
}
foo:
leaq4(%rdi), %rax
# %fs:4(%rdi) (%rax)
# %fs:(%rdi) (%rdi)
ret
Alexander
Re: typeof and operands in named address spaces
On Thu, 5 Nov 2020, Alexander Monakov via Gcc wrote:
> On Thu, 5 Nov 2020, Uros Bizjak via Gcc wrote:
>
> > > No, this is not how LEA operates. It needs a memory input operand. The
> > > above will report "operand type mismatch for 'lea'" error.
> >
> > The following will work:
> >
> > asm volatile ("lea (%1), %0" : "=r"(addr) : "r"((uintptr_t)&x));
>
> This is the same as a plain move though, and the cast to uintptr_t doesn't
> do anything, you can simply pass "r"(&x) to the same effect.
>
> The main advantage of passing a "fake" memory location for use with lea is
> avoiding base+offset computation outside the asm. If you're okay with one
> extra register tied up by the asm, just pass the address to the asm directly:
>
> void foo(__seg_fs int *x)
> {
> asm("# %0 (%1)" :: "m"(x[1]), "r"(&x[1]));
> asm("# %0 (%1)" :: "m"(x[0]), "r"(&x[0]));
> }
Actually, in the original context the asm ties up %rsi no matter what (because
the operand must be in %rsi to make the call), so the code would just
pass "S"(&var) for the call alternative and "m"(var) for the native instruction.
Then the only disadvantage is useless mov/lea to %rsi on the common path when
the alternative selected at runtime is native.
Alexander
Re: typeof and operands in named address spaces
On Thu, Nov 5, 2020 at 2:39 PM Alexander Monakov wrote:
>
> On Thu, 5 Nov 2020, Alexander Monakov via Gcc wrote:
>
> > On Thu, 5 Nov 2020, Uros Bizjak via Gcc wrote:
> >
> > > > No, this is not how LEA operates. It needs a memory input operand. The
> > > > above will report "operand type mismatch for 'lea'" error.
> > >
> > > The following will work:
> > >
> > > asm volatile ("lea (%1), %0" : "=r"(addr) : "r"((uintptr_t)&x));
> >
> > This is the same as a plain move though, and the cast to uintptr_t doesn't
> > do anything, you can simply pass "r"(&x) to the same effect.
> >
> > The main advantage of passing a "fake" memory location for use with lea is
> > avoiding base+offset computation outside the asm. If you're okay with one
> > extra register tied up by the asm, just pass the address to the asm
> > directly:
> >
> > void foo(__seg_fs int *x)
> > {
> > asm("# %0 (%1)" :: "m"(x[1]), "r"(&x[1]));
> > asm("# %0 (%1)" :: "m"(x[0]), "r"(&x[0]));
> > }
>
> Actually, in the original context the asm ties up %rsi no matter what (because
> the operand must be in %rsi to make the call), so the code would just
> pass "S"(&var) for the call alternative and "m"(var) for the native
> instruction.
Or pass both, "m"(var), and
uintptr_t *p = (uintptr_t *)(uintptr_t) &var;
"m"(*p) alternatives, similar to what is done in the original patch.
The copy to %rsi can then be a part of the alternative assembly.
Uros.
Re: typeof and operands in named address spaces
> On Nov 5, 2020, at 4:26 AM, Uros Bizjak wrote:
>
> On Thu, Nov 5, 2020 at 1:14 PM Alexander Monakov wrote:
>
>>> I was also thinking of introducing of operand modifier, but Richi
>>> advises the following:
>>>
>>> --cut here--
>>> typedef __UINTPTR_TYPE__ uintptr_t;
>>>
>>> __seg_fs int x;
>>>
>>> uintptr_t test (void)
>>> {
>>> uintptr_t *p = (uintptr_t *)(uintptr_t) &x;
>>> uintptr_t addr;
>>>
>>> asm volatile ("lea %1, %0" : "=r"(addr) : "m"(*p));
>>>
>>> return addr;
>>> }
>>
>> This is even worse undefined behavior compared to my solution above:
>> this code references memory in uintptr_t type, while mine preserves the
>> original type via __typeof. So this can visibly break with TBAA (though
>> the kernel uses -fno-strict-aliasing, so this particular concern wouldn't
>> apply there).
>
> Agreed, but I was trying to solve this lone use case in the kernel. It
> fits this particular usage, so I found a bit of overkill to implement
> the otherwise useless operand modifier in gcc. As discussed
> previously, these hacks are needed exclusively in asm templates, they
> are not needed in "normal" C code.
>>
>> If you don't care about preserving sizeof and type you can use a cast to
>> char:
>>
>> #define strip_as(mem) (*(char *)(intptr_t)&(mem))
>
> I hope that a developer from kernel can chime in and express their
> opinion on the proposed approaches.
>
I haven’t looked all that closely at precisely what the kernel needs,
but I’ve had bad experiences with passing imprecise things into asm
“m” and “=m” operands. GCC seems to assume, quite reasonably, that if
I pass a value via “m” or “=m”, then I read or write *that value*.
So, if we use type hackery to produce an lvalue or rvalue that has the
address space stripped, then I would imagine I get UB — GCC will try
to understand what value I’m reading or writing, and this will only
match what I’m actually doing by luck.
It’s kind of like doing this (sorry for whitespace damage):
int read_int(int *ptr)
{
int ret; uintptr_t tmp;
asm (
"lea %[val], %[tmp]\n\t"
"mov 4(%[tmp]), %[ret]"
: [ret] "=r" (ret), [tmp] "+r" (tmp)
: [val] "m" (*(ptr - 1)));
return ret;
}
That code is obviously rather contrived, but I think it's
fundamentally the same type of hack as all these typeofs. I haven't
tested precisely what GCC does, but I suspect we have:
int foo;
read_int(&foo); // UB
int foo[2];
read_int(foo[1]); // Maybe UB, but maybe non-UB that returns garbage
So I think a better constraint type would be an improvement. Or maybe
a more general "pointer" constraint could be invented for this and
other use cases:
[name] "p" (ptr)
With this constraint, ptr must be uintptr_t or intptr_t. %[name]
refers to ptr, formatted as a dereference operation. So the generated
asm is identical to [name] "m" (*(char *)ptr), but the semantics are
different. The problem is that I don't know how to specify the
semantics, but at least the instant UB of building and dereferencing a
garbage pointer would be avoided.
--Andy
gcc-8-20201105 is now available
Snapshot gcc-8-20201105 is now available on https://gcc.gnu.org/pub/gcc/snapshots/8-20201105/ and on various mirrors, see http://gcc.gnu.org/mirrors.html for details. This snapshot has been generated from the GCC 8 git branch with the following options: git://gcc.gnu.org/git/gcc.git branch releases/gcc-8 revision 6739ef0748ce47b937563aaa8098d9b3a19bb3b7 You'll find: gcc-8-20201105.tar.xzComplete GCC SHA256=08b102660450595dd3329880a51bdf2758853434fd1eccdecb375c83248903e2 SHA1=3755172724074968c0ad39f29d3758347eb30682 Diffs from 8-20201029 are available in the diffs/ subdirectory. When a particular snapshot is ready for public consumption the LATEST-8 link is updated and a message is sent to the gcc list. Please do not use a snapshot before it has been announced that way.
Re: Symbol + Constant output.
Hi,
I've adjust the testcase and now it only contains constant offset, since
with -fPIC the mov target address does not contain any symbol in the assembler.
Could you help to check the attached changes on darwin and see if they
all get passed?
(I still can not build darwin target currently)
Thanks.
Iain Sandoe 于2020年11月4日周三 下午5:19写道：
>
> Hi.
>
> Hongyu Wang wrote:
>
> >> 3. are you intending to update the tests?
> >
> > Yes, so could you tell me what does missing “_” means? I have some
> > trouble building darwin target for now.
>
> Darwin uses a USER_LABEL_PREFIX of ‘_’ (there are a small number of targets
> that do this).
>
> So public symbols begin with _
>
> In the case that you match like:
>
> …. ^\n]*%xmm0[^\n\r]*k1
>
> there’s no need to make any amendment (since the _ is covered by [^\n\r]).
>
> if you need to match 16+k1 … then for targets using USER_LABEL_PREFIX,
> it would need to be 16+_?k1 (so that it matches _k1 for them and k1 for
> Linux)
>
> OK?
>
> (if you want me to test a potential patch on Darwin, that’s also fine).
>
> >> As for the comments on the asm output.
> >>
> >> 1) it would seem that both comments can’t be correct (since they
> >> contradict!)
> >> 2) AFAICT, None of the assemblers I use has any issue with either order
> >> 3) perhaps there’s no assembler in use that cares any more
> >> 4) clang produces symbol+offset for that case on Darwin (i.e. the same as
> >> final.c).
> >
> > That means the i386.c part should align with final.c, but I can't make the
> > decision, and I'm not sure if there is more failure in x86 tests with this
> > change.
>
> agreed, it would need wide testing, and perhaps not urgent at this moment but
> it would be nice to make things consistent; it helps with maintainance.
>
> Iain
>
--
Regards,
Hongyu, Wang
From 9009ce97099b3a80fdf61a1927c1fff9c7f5b9bf Mon Sep 17 00:00:00 2001
From: hongyuw1
Date: Fri, 6 Nov 2020 15:08:10 +0800
Subject: [PATCH] Adjust Keylocker regex pattern for darwin, and add missing
aesenc256kl test.
gcc/testsuite/ChangeLog
* gcc.target/i386/keylocker-aesdec128kl.c: Adjust regex patterns.
* gcc.target/i386/keylocker-aesdec256kl.c: Likewise.
* gcc.target/i386/keylocker-aesdecwide128kl.c: Likewise.
* gcc.target/i386/keylocker-aesdecwide256kl.c: Likewise.
* gcc.target/i386/keylocker-aesenc128kl.c: Likewise.
* gcc.target/i386/keylocker-aesencwide128kl.c: Likewise.
* gcc.target/i386/keylocker-aesencwide256kl.c: Likewise.
* gcc.target/i386/keylocker-encodekey128.c: Likewise.
* gcc.target/i386/keylocker-encodekey256.c: Likewise.
* gcc.target/i386/keylocker-aesenc256kl.c: New test.
---
.../gcc.target/i386/keylocker-aesdec128kl.c | 8 ++---
.../gcc.target/i386/keylocker-aesdec256kl.c | 8 ++---
.../i386/keylocker-aesdecwide128kl.c | 36 +--
.../i386/keylocker-aesdecwide256kl.c | 36 +--
.../gcc.target/i386/keylocker-aesenc128kl.c | 8 ++---
.../gcc.target/i386/keylocker-aesenc256kl.c | 17 +
.../i386/keylocker-aesencwide128kl.c | 36 +--
.../i386/keylocker-aesencwide256kl.c | 36 +--
.../gcc.target/i386/keylocker-encodekey128.c | 14
.../gcc.target/i386/keylocker-encodekey256.c | 18 +-
10 files changed, 117 insertions(+), 100 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/i386/keylocker-aesenc256kl.c
diff --git a/gcc/testsuite/gcc.target/i386/keylocker-aesdec128kl.c b/gcc/testsuite/gcc.target/i386/keylocker-aesdec128kl.c
index 3cdda8ed7b0..9c3c8a88b0e 100644
--- a/gcc/testsuite/gcc.target/i386/keylocker-aesdec128kl.c
+++ b/gcc/testsuite/gcc.target/i386/keylocker-aesdec128kl.c
@@ -1,9 +1,9 @@
/* { dg-do compile } */
/* { dg-options "-mkl -O2" } */
-/* { dg-final { scan-assembler "movdqa\[ \\t\]+\[^\n\]*k2\[^\n\r]*%xmm0" } } */
-/* { dg-final { scan-assembler "aesdec128kl\[ \\t\]+\[^\n\]*h1\[^\n\r]*%xmm0" } } */
-/* { dg-final { scan-assembler "sete" } } */
-/* { dg-final { scan-assembler "(?:movdqu|movups)\[ \\t\]+\[^\n\]*%xmm0\[^\n\r]*k1" } } */
+/* { dg-final { scan-assembler {movdqa[ \t]+[^\n\r]*, %xmm0} } } */
+/* { dg-final { scan-assembler {aesdec128kl[ \t]+[^\n\r]*, %xmm0} } } */
+/* { dg-final { scan-assembler {sete} } } */
+/* { dg-final { scan-assembler {(?:movdqu|movups)[ \t]+[^\n\r]*%xmm0,[^\n\r]*} } } */
#include
diff --git a/gcc/testsuite/gcc.target/i386/keylocker-aesdec256kl.c b/gcc/testsuite/gcc.target/i386/keylocker-aesdec256kl.c
index 70b2c6357fa..6012b69e9bf 100644
--- a/gcc/testsuite/gcc.target/i386/keylocker-aesdec256kl.c
+++ b/gcc/testsuite/gcc.target/i386/keylocker-aesdec256kl.c
@@ -1,9 +1,9 @@
/* { dg-do compile } */
/* { dg-options "-mkl -O2" } */
-/* { dg-final { scan-assembler "movdqa\[ \\t\]+\[^\n\]*k2\[^\n\r]*%xmm0" } } */
-/* { dg-final { scan-assembler "aesdec256kl\[ \\t\]+\[^\n\]*h1\[^\n\r]*%xmm0" } } */
-/* { dg-final { scan-assembler "sete" } } */
-/* { dg-final { scan-assembler "(?:movdqu|movups)\[ \\t\]+\[^\n\]*%xmm0\
