replace AA runtime hooks with templated implementation#21066
replace AA runtime hooks with templated implementation#21066thewilsonator merged 29 commits intodlang:masterfrom
Conversation
|
Thanks for your pull request, @rainers! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
|
|
Thank you for working on this. |
rainers
force-pushed
the
tmpl_aa
branch
3 times, most recently
from
bb48686 to
9d998e3
Compare
rainers
force-pushed
the
tmpl_aa
branch
3 times, most recently
from
536a1ca to
ea07044
Compare
rainers
force-pushed
the
tmpl_aa
branch
3 times, most recently
from
8fdf93d to
4521625
Compare
rainers
force-pushed
the
tmpl_aa
branch
3 times, most recently
from
261f84d to
3a3faac
Compare
rainers
force-pushed
the
tmpl_aa
branch
5 times, most recently
from
e31b9fb to
e339e39
Compare
rainers
force-pushed
the
tmpl_aa
branch
4 times, most recently
from
f5d5afb to
8705d8a
Compare
|
This now passes all the tests with all associative array operations templated and the runtime implementation removed. It is probably a bit large for review, though. I have rebased and split off a couple of preliminary commits, some cherry picking
|
Not sure what you mean. With the empty object.d I get If you mean a regular build with object.d from druntime: the backend no longer needs to deal with AA operations anymore. |
Correct, it is not meant to be valid. https://gcc.gnu.org/pr100967 |
Before this change: After: The problem seems to be that now the use of RTTI is deep within some template, and it's not possible to get the instantiation trace from the C++ side of the compiler interface. I can expose this function as https://github.com/dlang/dmd/blob/master/compiler/src/dmd/dtemplate.d#L962-L963 |
|
Ah, ok. entryTI and a couple more fields are only kept for binary compatibility and could be removed altogether, including the typeid-assignment: Would that help? An array of buckets is allocated later, normally using typeid, too. Is this changed for -nortti somehow? Does gdb know about the specific layout of the AA impl struct or would a change be ok? I think the mago debug engine would need to be adapted if it is changed. |
|
All the AA details should be opaque to the user. We should be able to change them at any time, as long as the public API is the same. |
|
The test in gdc was for checking that we get a friendly error and no ICE/segfault when compiling with Now the error is incoherent because rtti use is disconnected from the user code that triggered the template instantiation (this is also a good thing as a druntime implementation could eschew rtti). Templates are the inverse of opaque, if something goes wrong in the details of the template because user code did something that wasn't expected, then it's innards will always be exposed. |
Maybe we can detect lowerings in printInstantiationTrace() and skip anything between the error and the location of the lowering. These might be stacked aswell, though. |
|
@rainers - Possibly, though as a primer have added printInstantiationTrace to the C++ interface anyway so that gdc (and possibly ldc) can link errors back to the originating source code. #22139 I think the second part along with what's already done is to adjust the test my end so that it is self contained again - the original tests strictly had no druntime dependencies, even though they didn't have // { dg-do compile }
// { dg-options "-fno-rtti" }
// { dg-shouldfail "expressions depend on TypeInfo" }
int* testInExp(int key, int[int] aa)
{
return key in aa; // { dg-error "requires 'object.TypeInfo' and cannot be used with '-fno-rtti'" }
}Now they require extra boilerplate added to remain without druntime dependencies. For example: // { dg-do compile }
// { dg-options "-fno-rtti" }
// { dg-shouldfail "expressions depend on TypeInfo" }
module object;
class Object {}
class TypeInfo {}
struct AA(K, V)
{
this(int sz) nothrow
{
keyTI = typeid(K); // { dg-error "'object.TypeInfo' cannot be used with '-fno-rtti'" }
}
TypeInfo keyTI;
}
auto _d_aaIn(T : V[K], K, V, K2)(inout T a, auto ref scope K2 key)
{
auto aa = *(cast(inout(AA!(K, V))*)&a); // { dg-note "instantiated from here" }
return null;
}
int* testInExp(int key, int[int] aa)
{
return key in aa; // { dg-note "instantiated from here" }
}Given that many other hooks previously dealt with by the glue/backend have been moved to frontend, I ought to check that other similar assumptions aren't made by other lowerings that would cause ICE/segfault when druntime code is missing or incomplete. |
I didn't mean it that way. I meant that the user shouldn't care how the innards work. So changing the template to remove the setting of Obviously, they should compile, or provide a good error message if they don't. Ideally this is solved in the library with static asserts, but if needed the compiler can get involved. |
|
@rainers looks like this work unearthed a latent bug in https://compiler-explorer.com/z/ovTfzYr11 Frontend passes wrong AST to codegen (not introduced by this or AA related). |
|
I guess this PR caused the following deprecation: void foo() @safe {
immutable key = 1;
int[int] aa;
aa[key] = 123;
}=> This broke the Phobos unittests for LDC, since LDC uses |
This boils down to this code causing the same deprecation message: void foo() @safe
{
immutable key = 1;
_x_aaGetY(cast(int)key);
}
void _x_aaGetY(K)(auto ref K key)
{
}I suspect Note that the cast was inserted by previous versions, too, but the ref was taken by the backend, so it was just accepted silently. I can remove the cast, but that might delay errors into the template instantiations. Let's see how this works out... |
|
Indeed auto ref picks ref for some reason. I don't know how this is valid. I guess casting between mutability does not for some reason turn an expression into an rvalue? Seems to have been introduced in 2.095.0 Prior to this, it picks non-ref. Seems like a bug that shouldn't affect this. Indeed, why is there a cast in the first place? Why are we using auto ref? So many questions... |
…able keys being cast to non-immutable. Avoid adding the cast for implicitly convertable tyypes.
…able keys being cast to non-immutable. Avoid adding the cast for implicitly convertable expressions.
This is to avoid copying the key as much as possible. There are even tests for this in newaa.d and druntime/test/aa/test_aa.d.
Without the cast a couple of tests fail. AFAICT these are mostly issues with value range propagation causing the key expression no longer implicitly casting to the key type. It seems to work if we avoid the cast for conversions that only change the constness: #22208 |
…keys being cast to non-immutable. (#22208) Avoid adding the cast for implicitly convertable expressions.
Is it just me, or does this sound like a really bad default for basic types? I would not expect to be using ref on |
I don't expect that to make much of a difference, especially in this case where the called function is rather elaborate. If the address needs to be calculated anyway and is passed in registers, the dereferencing is just moved into the function. If inlining kicks in, the optimizer might also elide all the differences. |
break change in AA runtime in dlang/dmd#21066
break change in AA runtime in dlang/dmd#21066 CI: drop userdata from wasm/wgpu runner
|
This pull request introduced a regression: |
|
@rainers I think this broke strict alignment targets (such as SPARC). :-( Going to attempt to get a proper backtrace. Edit: |
|
@rainers might have found the culprit. // NOTE: Returns pointer
_d_assocarrayliteralTX!(int, string)
{
pure nothrow @safe Impl!(int, string)* _d_assocarrayliteralTX(int[] keys, string[] vals)
{
// ...
}
}
// NOTE: ABI of aa parameter is: struct AA{void* ptr;}
values!(string, int)
{
auto pure nothrow @property @safe string[] values(inout(string[int]) aa)
{
return _aaValues(aa);
}
}
/// !!! BOOM: Passing pointer to function that accepts a struct.
void main()
{
string[] a = values(_d_assocarrayliteralTX([5], ["hello"]));
return 0;
}This "just works" on x86 as the following types: void* ptr;
struct aa { void* ptr; }Are passed around identically. This is not true for all ABIs, however. SPARC32 being one of them (they are passed by invisible reference), and I think ARM would be another too. This did not occur when the glue/codegen generated the call to a = values ({.ptr=_d_assocarrayliteralTX (...)}); |
|
@rainers probably best to deal with this in the glue/codegen layer, so that the AssocArrayLiteral visitor returns |
@ibuclaw the dmd glue layer doesn't really know about associative arrays anymore (but to assert on missing lowerings or to swtitch to it for literals), so I assume that it uses For a consistent AST it would be better to return |
12 participants
Adds the complete templated implementation to newaa.d, keeping binary compatibility with rt/aaa.d for now.
This should help inferring proper function attributes on the operations, aswell as using regular constructors/copy operation on key and value types.
Four of the six compiler hooks are replaced, but there are two more calls from TypeInfo_AssociativeArray's getHash() and equal() to forward.
Most of the time was spent fighting
inoutin template arguments :/