From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org Cc: igt-dev@lists.freedesktop.org Subject: Re: [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs Date: Fri, 24 May 2019 09:44:12 +0100 [thread overview] Message-ID: <9e387397-0541-7197-707e-2c9cca3d0d45@linux.intel.com> (raw) In-Reply-To: <155868683254.28319.251717788186809854@skylake-alporthouse-com> On 24/05/2019 09:33, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-05-24 09:20:47) >> >> On 24/05/2019 08:25, Chris Wilson wrote: >>> Apparently VLA structs (e.g. struct { int array[count] }) is a gcc >>> extension that clang refuses to support as handling memory layout is too >>> difficult for it. >>> >>> Move the on-stack VLA to the heap. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> benchmarks/gem_wsim.c | 146 +++++++++++++++++++++++++++--------------- >>> 1 file changed, 95 insertions(+), 51 deletions(-) >>> >>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c >>> index e2ffb93a9..0a0032bff 100644 >>> --- a/benchmarks/gem_wsim.c >>> +++ b/benchmarks/gem_wsim.c >>> @@ -1441,6 +1441,48 @@ set_ctx_sseu(struct ctx *ctx, uint64_t slice_mask) >>> return slice_mask; >>> } >>> >>> +static size_t sizeof_load_balance(int count) >>> +{ >>> + struct i915_context_engines_load_balance *ptr; >>> + >>> + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0])); >> >> This seems wrong - is bound to trigger. > > Why does it seem wrong? That's the calculation used previously, and the > ptr->engines[] was meant to be packed in order for > sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]). Anyway, > I threw it in there to check if the calculation was sane. Because sizeof(ptr->engines[0]) == sizeof(ptr->engines[N]), since the code is not declaring N big array, just referencing the element N. So for more than one engine I expect it explodes. Unless I am way wrong.. I guess someone needs to run it.. :) >>> + return sizeof(*ptr) + sizeof(ptr->engines[count]); >> >> So size of of engine needs to be multiplied by count. > > (Just note this is the what the current VLA evaluates to :) > >>> +} >>> + >>> +static struct i915_context_engines_load_balance * >>> +alloc_load_balance(int count) >>> +{ >>> + return calloc(1, sizeof_load_balance(count)); >> >> How about alloca so cleanup is simpler? Or is alloca also on the >> unpopular list? > > I don't mind. Would shave a few lines indeed, but we need the memsets > back. #define alloca0()? And a helper macro to generically deal with struct header + engines array so it doesn't need to be repeated three times. Yadayada too much work.. :) ... >> Or possibly what Simon suggested, just a large temporary stack arrays >> would be enough and easiest diff. Just with an assert that it fits. > > I don't think that is as clean for the long term. ... this should be just fine for now so I'd vote for it. Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
WARNING: multiple messages have this Message-ID (diff)
From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org Cc: igt-dev@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs Date: Fri, 24 May 2019 09:44:12 +0100 [thread overview] Message-ID: <9e387397-0541-7197-707e-2c9cca3d0d45@linux.intel.com> (raw) In-Reply-To: <155868683254.28319.251717788186809854@skylake-alporthouse-com> On 24/05/2019 09:33, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-05-24 09:20:47) >> >> On 24/05/2019 08:25, Chris Wilson wrote: >>> Apparently VLA structs (e.g. struct { int array[count] }) is a gcc >>> extension that clang refuses to support as handling memory layout is too >>> difficult for it. >>> >>> Move the on-stack VLA to the heap. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> benchmarks/gem_wsim.c | 146 +++++++++++++++++++++++++++--------------- >>> 1 file changed, 95 insertions(+), 51 deletions(-) >>> >>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c >>> index e2ffb93a9..0a0032bff 100644 >>> --- a/benchmarks/gem_wsim.c >>> +++ b/benchmarks/gem_wsim.c >>> @@ -1441,6 +1441,48 @@ set_ctx_sseu(struct ctx *ctx, uint64_t slice_mask) >>> return slice_mask; >>> } >>> >>> +static size_t sizeof_load_balance(int count) >>> +{ >>> + struct i915_context_engines_load_balance *ptr; >>> + >>> + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0])); >> >> This seems wrong - is bound to trigger. > > Why does it seem wrong? That's the calculation used previously, and the > ptr->engines[] was meant to be packed in order for > sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]). Anyway, > I threw it in there to check if the calculation was sane. Because sizeof(ptr->engines[0]) == sizeof(ptr->engines[N]), since the code is not declaring N big array, just referencing the element N. So for more than one engine I expect it explodes. Unless I am way wrong.. I guess someone needs to run it.. :) >>> + return sizeof(*ptr) + sizeof(ptr->engines[count]); >> >> So size of of engine needs to be multiplied by count. > > (Just note this is the what the current VLA evaluates to :) > >>> +} >>> + >>> +static struct i915_context_engines_load_balance * >>> +alloc_load_balance(int count) >>> +{ >>> + return calloc(1, sizeof_load_balance(count)); >> >> How about alloca so cleanup is simpler? Or is alloca also on the >> unpopular list? > > I don't mind. Would shave a few lines indeed, but we need the memsets > back. #define alloca0()? And a helper macro to generically deal with struct header + engines array so it doesn't need to be repeated three times. Yadayada too much work.. :) ... >> Or possibly what Simon suggested, just a large temporary stack arrays >> would be enough and easiest diff. Just with an assert that it fits. > > I don't think that is as clean for the long term. ... this should be just fine for now so I'd vote for it. Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-05-24 8:44 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-24 7:25 [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs Chris Wilson 2019-05-24 7:25 ` [igt-dev] " Chris Wilson 2019-05-24 7:45 ` Ser, Simon 2019-05-24 7:45 ` Ser, Simon 2019-05-24 7:50 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork 2019-05-24 8:20 ` [PATCH i-g-t] " Tvrtko Ursulin 2019-05-24 8:20 ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin 2019-05-24 8:27 ` [igt-dev] " Ser, Simon 2019-05-24 8:27 ` [igt-dev] [Intel-gfx] " Ser, Simon 2019-05-24 8:33 ` Chris Wilson 2019-05-24 8:33 ` [igt-dev] [Intel-gfx] " Chris Wilson 2019-05-24 8:39 ` [igt-dev] " Ser, Simon 2019-05-24 8:39 ` [igt-dev] [Intel-gfx] " Ser, Simon 2019-05-24 8:44 ` Tvrtko Ursulin [this message] 2019-05-24 8:44 ` Tvrtko Ursulin 2019-05-24 8:45 ` [PATCH i-g-t v2] benchmarks/gem_wsim: Manually calculate VLA struct sizes Chris Wilson 2019-05-24 8:45 ` [igt-dev] " Chris Wilson 2019-05-24 9:35 ` Tvrtko Ursulin 2019-05-24 9:35 ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin 2019-05-24 11:07 ` [igt-dev] ✓ Fi.CI.BAT: success for benchmarks/gem_wsim: Heap allocate VLA structs (rev2) Patchwork 2019-05-25 13:11 ` [igt-dev] ✓ Fi.CI.IGT: success for benchmarks/gem_wsim: Heap allocate VLA structs Patchwork 2019-05-25 16:10 ` [igt-dev] ✓ Fi.CI.IGT: success for benchmarks/gem_wsim: Heap allocate VLA structs (rev2) Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=9e387397-0541-7197-707e-2c9cca3d0d45@linux.intel.com \ --to=tvrtko.ursulin@linux.intel.com \ --cc=chris@chris-wilson.co.uk \ --cc=igt-dev@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.