Issue 19946 - Add memset template function #3837

dkorpel · 2022-06-08T17:01:56Z

This allows dmd to lower array assignment to memset in the frontend instead of the glue layer.

dlang-bot · 2022-06-08T17:02:00Z

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✗	19946	regression	In betterC filling an array with a non-zero value fails for types of size > 1 due to missing _memset16/_memset32/etc.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3837"

maxhaton · 2022-06-08T20:38:27Z

src/rt/memset.d

+    array = array to set
+    value = value to set each array element to
+*/
+void _d_memset(T)(T[] array, T value)


const on the RHS?

maxhaton · 2022-06-08T20:38:36Z

src/rt/memset.d

@@ -24,6 +24,36 @@ extern (C)

 extern (C):

+/**
+Set all elements of an array to be a specified value.


Suggested change

Set all elements of an array to be a specified value.

Set all elements of an array to a specified value.

ibuclaw

core.internal.memset?

IIRC this rt module is not compilable due to dmd-specific assembly.

maxhaton · 2022-06-09T07:50:50Z

core.internal.memset?

IIRC this rt module is not compilable due to dmd-specific assembly.

This is dmd specific unless you want your lowering to gain a function call

ibuclaw · 2022-06-09T11:17:48Z

lower array assignment to memset in the frontend instead of the glue layer.

Doesn't sound dmd-specific to me.

maxhaton · 2022-06-09T11:42:40Z

lower array assignment to memset in the frontend instead of the glue layer.

Doesn't sound dmd-specific to me.

It should be dmd specific, put it that way. It can be made optional with a frontend param.

RazvanN7 · 2022-06-09T11:56:29Z

We can just move the function to a different file, but it really doesn't matter since the template instance will go in the resulted object file.

ibuclaw · 2022-06-09T15:16:17Z

We can just move the function to a different file, but it really doesn't matter since the template instance will go in the resulted object file.

It does matter if you can't compile the module it's located in.

maxhaton · 2022-06-09T15:55:51Z

We can just move the function to a different file, but it really doesn't matter since the template instance will go in the resulted object file.

It does matter if you can't compile the module it's located in.

The module what is contained in? The whole point of this is that dmd errors with betterC because it assumes some code is present when its not, hence a template.

kinke · 2022-06-09T16:25:38Z

How can this work in the first place? The rt.* modules aren't deployed by DMD releases, so cannot be imported.

IMO, these dummy-templatizations for -betterC compatibility are absolutely terrible. In an ideal world, the druntime module interdependencies would be carefully laid out, so that some -betterC project can still link druntime without bloating the binary too much. E.g., for these DMD-backend-specific memset helpers, one should only need the memset.o object file from the static druntime lib, done.

ibuclaw · 2022-06-09T17:28:59Z

We can just move the function to a different file, but it really doesn't matter since the template instance will go in the resulted object file.

It does matter if you can't compile the module it's located in.

The module what is contained in? The whole point of this is that dmd errors with betterC because it assumes some code is present when its not, hence a template.

memset.d:40:17: error: found ‘EDI’ when expecting ‘:’
   40 |         mov     EDI,p           ;
      |                 ^
memset.d:40:20: error: expected constant string constraint for operand, not ‘,’
   40 |         mov     EDI,p           ;
      |                    ^

Yada, yada.

maxhaton · 2022-06-09T18:03:15Z

We can just move the function to a different file, but it really doesn't matter since the template instance will go in the resulted object file.

It does matter if you can't compile the module it's located in.

The module what is contained in? The whole point of this is that dmd errors with betterC because it assumes some code is present when its not, hence a template.
memset.d:40:17: error: found ‘EDI’ when expecting ‘:’
   40 |         mov     EDI,p           ;
      |                 ^
memset.d:40:20: error: expected constant string constraint for operand, not ‘,’
   40 |         mov     EDI,p           ;
      |                    ^
Yada, yada.

The existing functions will be removed. Arguably the whole module then just move this into arrayop perhaps.

Note that arrayOp can nearly do memset

ljmf00 · 2022-06-10T09:15:05Z

src/rt/memset.d

+void _d_memset(T)(T[] array, T value)
+{
+    foreach (i; 0 .. array.length)
+        array[i] = value;


Why this inefficient assignment? Please fallback to the actual memset

The idea is to start with something simple that works. memset doesn't support multi-byte values and is not available at CTFE.

The idea is to start with something simple that works. memset doesn't support multi-byte values and is not available at CTFE.

Ah sure, I understand. There's already intrinsics (below) to do multi-byte assignment, tho. But yeah, I agree that the complete version should be done separately as it might require a more complex/complete implementation.

I'll leave a list of things we need to account for:

ellaborate assignment and CTFE (operator overloading, this implementation)

single byte assignment (straight memset call)

multi byte assignment (possibly a generic and inline assembly versions, would be cool to have a generic version with the __vector API)

For the byte assignments we need to check for immutable qualifiers or for copy ctors.

The compiler will automatically do this.

No one cares or should care about dmd performance, LLVM and GCC both go to some length to find these kinds of assignments and optimize them.

The compiler will automatically do this.

No one cares or should care about dmd performance, LLVM and GCC both go to some length to find these kinds of assignments and optimize them.

It does for single-byte assignment, although, for multiple bytes like 32-bit size, it does a poor job. See https://godbolt.org/z/994n163ao . I would also value not having such performance regression on DMD tho. We can probably do a cleaner generic implementation with the __vector API, but I agree doing those on separate PRs. I believe there is a huge performance benefit here. This is also a cool thing to propose on the LLVM optimization/transformation passes. I'm not sure how about cache/branch predictor would behave with this inline asm but I think it doesn't matter, given the generated assembly.

dkorpel · 2022-06-10T09:45:54Z

In an ideal world, the druntime module interdependencies would be carefully laid out, so that some -betterC project can still link druntime without bloating the binary too much. E.g., for these DMD-backend-specific memset helpers, one should only need the memset.o object file from the static druntime lib, done.

That's a good stand point, but currently:

betterC is defined as 'subset of D requiring only the C standard lib, not druntime'
assigning a value to an array requires druntime (when compiling with DMD)
people want to be able to assign values to an array in betterC without hassle (issue 19946 and duplicates)

We can't have all 3, and the template solution would remove point 2. You're either suggesting to remove point 3 by asking users to link memset.o, or point 1 when the compiler does it automatically. That's essentially rebranding betterC to be 'light druntime' instead of 'no druntime'. That might not be a bad idea, after all some users call -betterC 'worseD' or 'a cancer' and advocate for custom light druntimes instead. It would be a bigger commitment that requires approval from Walter and Atila though.

dkorpel · 2022-06-10T09:48:05Z

IIRC this rt module is not compilable due to dmd-specific assembly.

Is there a version other than version (D_InlineAsm_X86) that would make GDC ignore it instead?

RazvanN7 · 2022-07-05T11:54:05Z

@dkorpel what are your plans with this PR?

dkorpel · 2022-07-05T11:55:49Z

I think I'll follow Iain's suggestion and move it to core.internal.memset

RazvanN7 · 2022-07-08T08:02:07Z

So, if I understand correctly, you then need to:

create a dmd pr to replace the calls to memset with calls to arraySet
create a druntime pr to delete the old hooks

@ibuclaw @kinke do you oppose this course of action?

kinke · 2022-07-08T11:07:13Z

LDC currently inlines these nano loops (in its glue layer), so has no need for any frontend lowering.

ibuclaw · 2022-07-08T12:17:53Z

LDC currently inlines these nano loops (in its glue layer), so has no need for any frontend lowering.

Likewise here too. If you're going to lower array[] = n in the front-end anyway, at the very least:

Make this pragma(inline, true), to avoid any performance regression.
Probably don't want array[] = 0 lowered, as that's the most common case we already generate fast codegen for from the get go (without having to rely on the optimizer to understand what you mean).

maxhaton · 2022-07-08T12:47:06Z

LDC currently inlines these nano loops (in its glue layer), so has no need for any frontend lowering.

I'd be surprised this wasnt inlined (then unrolled) too. It's simple constant propagation to work out the loop is bounded.

I think the lowering should just be optional. dmd needs it because because injecting a loop into its IR tree is an unsolved problem, GDC and Ldc don't need it at all

Geod24 · 2022-07-09T22:46:52Z

Druntime have been merged into DMD. Please re-submit your PR to dlang/dmd repository.

dkorpel requested review from andralex and wilzbach as code owners June 8, 2022 17:01

maxhaton reviewed Jun 8, 2022

View reviewed changes

ibuclaw suggested changes Jun 9, 2022

View reviewed changes

ljmf00 reviewed Jun 10, 2022

View reviewed changes

dkorpel force-pushed the betterc-memset branch from b663692 to 2b82849 Compare July 5, 2022 13:12

dkorpel requested review from MartinNowak and schveiguy as code owners July 5, 2022 13:12

Issue 19946 - Add memset template function

1f84004

dkorpel force-pushed the betterc-memset branch from 2b82849 to 1f84004 Compare July 5, 2022 13:12

Geod24 closed this Jul 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 19946 - Add memset template function #3837

Issue 19946 - Add memset template function #3837

dkorpel commented Jun 8, 2022

dlang-bot commented Jun 8, 2022

maxhaton Jun 8, 2022

maxhaton Jun 8, 2022

ibuclaw left a comment •

edited

maxhaton commented Jun 9, 2022

ibuclaw commented Jun 9, 2022

maxhaton commented Jun 9, 2022

RazvanN7 commented Jun 9, 2022

ibuclaw commented Jun 9, 2022

maxhaton commented Jun 9, 2022

kinke commented Jun 9, 2022

ibuclaw commented Jun 9, 2022

maxhaton commented Jun 9, 2022

ljmf00 Jun 10, 2022

dkorpel Jun 10, 2022

ljmf00 Jun 10, 2022

maxhaton Jun 10, 2022

maxhaton Jun 10, 2022

ljmf00 Jun 13, 2022

dkorpel commented Jun 10, 2022

dkorpel commented Jun 10, 2022

RazvanN7 commented Jul 5, 2022

dkorpel commented Jul 5, 2022

RazvanN7 commented Jul 8, 2022

kinke commented Jul 8, 2022 •

edited

ibuclaw commented Jul 8, 2022

maxhaton commented Jul 8, 2022

Geod24 commented Jul 9, 2022

	Set all elements of an array to be a specified value.
	Set all elements of an array to a specified value.

Issue 19946 - Add memset template function #3837

Issue 19946 - Add memset template function #3837

Conversation

dkorpel commented Jun 8, 2022

dlang-bot commented Jun 8, 2022

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuclaw left a comment • edited

Choose a reason for hiding this comment

maxhaton commented Jun 9, 2022

ibuclaw commented Jun 9, 2022

maxhaton commented Jun 9, 2022

RazvanN7 commented Jun 9, 2022

ibuclaw commented Jun 9, 2022

maxhaton commented Jun 9, 2022

kinke commented Jun 9, 2022

ibuclaw commented Jun 9, 2022

maxhaton commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkorpel commented Jun 10, 2022

dkorpel commented Jun 10, 2022

RazvanN7 commented Jul 5, 2022

dkorpel commented Jul 5, 2022

RazvanN7 commented Jul 8, 2022

kinke commented Jul 8, 2022 • edited

ibuclaw commented Jul 8, 2022

maxhaton commented Jul 8, 2022

Geod24 commented Jul 9, 2022

ibuclaw left a comment •

edited

kinke commented Jul 8, 2022 •

edited