Skip to content

[netcore] Implement Object.GetRawData#17295

Closed
filipnavara wants to merge 1 commit intomono:masterfrom
filipnavara:objrawdata
Closed

[netcore] Implement Object.GetRawData#17295
filipnavara wants to merge 1 commit intomono:masterfrom
filipnavara:objrawdata

Conversation

@filipnavara
Copy link
Contributor

No description provided.

@filipnavara
Copy link
Contributor Author

I forgot there is intrinsic for it so it never gets executed but I think we can safely use the managed code instead.

@vargaz
Copy link
Contributor

vargaz commented Oct 11, 2019

Note that these kinds of code only have the same perf if you have an optimizing compiler like llvm or the coreclr jit, it will be slower with the mono JIT or interpreter. So we should keep the intrinsics.

@filipnavara
Copy link
Contributor Author

I checked the codegen and in both cases it ends up long_add_imm %rax <- %r15 [16] but the managed code generated an explicit null check for some reason.

@filipnavara
Copy link
Contributor Author

Ok, the NULL check comes from here:

mono/mono/mini/method-to-ir.c

Lines 9265 to 9268 in 9cc5ea3

if (sp [0]->type == STACK_OBJ) {
MONO_EMIT_NEW_BIALU_IMM (cfg, OP_COMPARE_IMM, -1, sp [0]->dreg, 0);
MONO_EMIT_NEW_COND_EXC (cfg, EQ, "NullReferenceException");
}

@filipnavara
Copy link
Contributor Author

The null check actually gets eliminated when the code gets inlined on the sites so it should work even in non-LLVM case.


[Intrinsic]
internal ref byte GetRawData () => throw new NotImplementedException ();
internal ref byte GetRawData () => ref Unsafe.As<Mono.RawData>(this).Data;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in favour of this. We have Intrinsic why to have managed code and dependencies which will never be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to remove the intrinsic in favour of the managed code. It seems to produce the same codegen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's worth it, as we already have the instrincis and the managed code is not really improving the readability or performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I beg to differ on the readability but I am fine with closing this as the improvement is questionable.

Copy link
Contributor Author

@filipnavara filipnavara Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we can share the implementation with CoreCLR which uses basically the same code as I used here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the interpreter keeping up with the intrinsics?

Copy link
Contributor Author

@filipnavara filipnavara Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this particular case the interpreter has both object.GetRawData (netcore only) and Unsafe.As. Unsafe.As is NOP in interpreter so it's quite cheap overall.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean performance, but in recognition.
Having the IL loosens the requirement of keeping up with recognition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants