Add support for 4-arg ops in mini IR #48908
Conversation
|
|
||
| struct MonoInst { | ||
| guint16 opcode; | ||
| guint8 type; /* stack type */ | ||
| guint8 flags; | ||
|
|
||
| /* used by the register allocator */ | ||
| gint32 dreg, sreg1, sreg2, sreg3; | ||
| gint32 dreg, sreg1, sreg2, sreg3, sreg4; |
There was a problem hiding this comment.
There was a fair bit of discussion about this when I looked into it, and I thought the conclusion was that we didn't want to add them this way since it's only an immediate value.
There was a problem hiding this comment.
This adds a lot of overhead for the tiny minority of opcodes which have 4 opcodes.
There was a problem hiding this comment.
There's 4 bytes of padding between sreg3 and next on 64-bit machines.
There was a problem hiding this comment.
We still run on a (sadly) 32-bit platform – wasm – so I don't think we can assume this doesn't add overhead on platforms we care about.
Which is not to say we can't take this approach, just that it shouldn't be predicated on incorrect assumptions.
| @@ -717,15 +717,15 @@ typedef struct { | |||
| LLVMArgInfo args [1]; | |||
| } LLVMCallInfo; | |||
|
|
|||
| #define MONO_MAX_SRC_REGS 3 | |||
| #define MONO_MAX_SRC_REGS 4 | |||
There was a problem hiding this comment.
I believe this is actually not enough and the 3 value is hard-coded in a few different places instead of just using this define.
There was a problem hiding this comment.
This PR is far away from being completed. I just did the most basic change. Will fix the errors exposed by testing.
|
Current consensus is 'too much churn/work for minimal gain'. #48475 (comment) |
ghost
locked as resolved and limited conversation to collaborators
Fixes #48475