Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for 4-arg ops in mini IR #48908

Closed
wants to merge 1 commit into from
Closed

Add support for 4-arg ops in mini IR #48908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/mono/mono/mini/genmdesc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,12 @@ def parse_mini_ops (target_define):
if m != None:
opcodes [m.group (1)] = OpDef(opcode_id, m.group (1), m.group (2), m.group (3), m.group (4))
else:
print ("Unable to parse line: '{0}'".format (line))
exit (1)
m = re.search (r"MINI_OP4\(\w+\s*\,\s*\"([^\"]+)\", (\w+), (\w+), (\w+), (\w+), (\w+)\)", line)
if m != None:
opcodes [m.group (1)] = OpDef(opcode_id, m.group (1), m.group (2), m.group (3), m.group (4))
else:
print ("Unable to parse line: '{0}'".format (line))
exit (1)
opcode_id += 1
opcode_file.close ()
return opcodes
Expand Down
9 changes: 9 additions & 0 deletions src/mono/mono/mini/helpers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,29 +25,38 @@
#ifdef MINI_OP3
#undef MINI_OP3
#endif
#ifdef MINI_OP4
#undef MINI_OP4
#endif

// This, instead of an array of pointers, to optimize away a pointer and a relocation per string.
#define MSGSTRFIELD(line) MSGSTRFIELD1(line)
#define MSGSTRFIELD1(line) str##line
static const struct msgstr_t {
#define MINI_OP(a,b,dest,src1,src2) char MSGSTRFIELD(__LINE__) [sizeof (b)];
#define MINI_OP3(a,b,dest,src1,src2,src3) char MSGSTRFIELD(__LINE__) [sizeof (b)];
#define MINI_OP4(a,b,dest,src1,src2,src3,src4) char MSGSTRFIELD(__LINE__) [sizeof (b)];
#include "mini-ops.h"
#undef MINI_OP
#undef MINI_OP3
#undef MINI_OP4
} opstr = {
#define MINI_OP(a,b,dest,src1,src2) b,
#define MINI_OP3(a,b,dest,src1,src2,src3) b,
#define MINI_OP4(a,b,dest,src1,src2,src3,src4) b,
#include "mini-ops.h"
#undef MINI_OP
#undef MINI_OP3
#undef MINI_OP4
};
static const gint16 opidx [] = {
#define MINI_OP(a,b,dest,src1,src2) offsetof (struct msgstr_t, MSGSTRFIELD(__LINE__)),
#define MINI_OP3(a,b,dest,src1,src2,src3) offsetof (struct msgstr_t, MSGSTRFIELD(__LINE__)),
#define MINI_OP4(a,b,dest,src1,src2,src3,src4) offsetof (struct msgstr_t, MSGSTRFIELD(__LINE__)),
#include "mini-ops.h"
#undef MINI_OP
#undef MINI_OP3
#undef MINI_OP4
};

#endif /* DISABLE_LOGGING */
Expand Down
11 changes: 9 additions & 2 deletions src/mono/mono/mini/method-to-ir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,12 @@ static GENERATE_GET_CLASS_WITH_CACHE (geqcomparer, "System.Collections.Generic",
#ifdef MINI_OP3
#undef MINI_OP3
#endif
#define MINI_OP(a,b,dest,src1,src2) dest, src1, src2, ' ',
#define MINI_OP3(a,b,dest,src1,src2,src3) dest, src1, src2, src3,
#ifdef MINI_OP4
#undef MINI_OP4
#endif
#define MINI_OP(a,b,dest,src1,src2) dest, src1, src2, ' ', ' ',
#define MINI_OP3(a,b,dest,src1,src2,src3) dest, src1, src2, src3, ' ',
#define MINI_OP4(a,b,dest,src1,src2,src3,src4) dest, src1, src2, src3, src4,
#define NONE ' '
#define IREG 'i'
#define FREG 'f'
Expand All @@ -220,9 +224,11 @@ mini_ins_info[] = {
};
#undef MINI_OP
#undef MINI_OP3
#undef MINI_OP4

#define MINI_OP(a,b,dest,src1,src2) ((src2) != NONE ? 2 : ((src1) != NONE ? 1 : 0)),
#define MINI_OP3(a,b,dest,src1,src2,src3) ((src3) != NONE ? 3 : ((src2) != NONE ? 2 : ((src1) != NONE ? 1 : 0))),
#define MINI_OP4(a,b,dest,src1,src2,src3,src4) ((src4) != NONE ? 4 : ((src3) != NONE ? 3 : ((src2) != NONE ? 2 : ((src1) != NONE ? 1 : 0)))),
/*
* This should contain the index of the last sreg + 1. This is not the same
* as the number of sregs for opcodes like IA64_CMP_EQ_IMM.
Expand All @@ -232,6 +238,7 @@ const gint8 mini_ins_sreg_counts[] = {
};
#undef MINI_OP
#undef MINI_OP3
#undef MINI_OP4

guint32
mono_alloc_ireg (MonoCompile *cfg)
Expand Down
9 changes: 7 additions & 2 deletions src/mono/mono/mini/mini-llvm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,12 @@ typedef struct {
#ifdef MINI_OP3
#undef MINI_OP3
#endif
#define MINI_OP(a,b,dest,src1,src2) dest, src1, src2, ' ',
#define MINI_OP3(a,b,dest,src1,src2,src3) dest, src1, src2, src3,
#ifdef MINI_OP4
#undef MINI_OP4
#endif
#define MINI_OP(a,b,dest,src1,src2) dest, src1, src2, ' ', ' ',
#define MINI_OP3(a,b,dest,src1,src2,src3) dest, src1, src2, src3, ' ',
#define MINI_OP4(a,b,dest,src1,src2,src3,src4) dest, src1, src2, src3, src4,
#define NONE ' '
#define IREG 'i'
#define FREG 'f'
Expand All @@ -225,6 +229,7 @@ mini_llvm_ins_info[] = {
};
#undef MINI_OP
#undef MINI_OP3
#undef MINI_OP4

#if TARGET_SIZEOF_VOID_P == 4
#define GET_LONG_IMM(ins) ((ins)->inst_l)
Expand Down
9 changes: 7 additions & 2 deletions src/mono/mono/mini/mini.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,15 +717,15 @@ typedef struct {
LLVMArgInfo args [1];
} LLVMCallInfo;

#define MONO_MAX_SRC_REGS 3
#define MONO_MAX_SRC_REGS 4
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@fanyang-mono fanyang-mono Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This PR is far away from being completed. I just did the most basic change. Will fix the errors exposed by testing.


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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

This adds a lot of overhead for the tiny minority of opcodes which have 4 opcodes.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's 4 bytes of padding between sreg3 and next on 64-bit machines.

Copy link
Contributor

@CoffeeFlux CoffeeFlux Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.


MonoInst *next, *prev;

Expand Down Expand Up @@ -1728,15 +1728,20 @@ extern MonoJitStats mono_jit_stats;
#ifdef MINI_OP3
#undef MINI_OP3
#endif
#ifdef MINI_OP4
#undef MINI_OP4
#endif
#define MINI_OP(a,b,dest,src1,src2) a,
#define MINI_OP3(a,b,dest,src1,src2,src3) a,
#define MINI_OP4(a,b,dest,src1,src2,src3,src4) a,
enum {
OP_START = MONO_CEE_LAST - 1,
#include "mini-ops.h"
OP_LAST
};
#undef MINI_OP
#undef MINI_OP3
#undef MINI_OP4

#if TARGET_SIZEOF_VOID_P == 8
#define OP_PCONST OP_I8CONST
Expand Down