On Tue, Apr 28, 2015 at 8:53 AM, Sergey Korshunoff <[email protected]> wrote:
> I removed all calls to vla_sp_save() keeping these only in
> decl_initializer(). All tests passed.
That's not saying very much—we don't have good test coverage for VLAs.
I think we can get away with simplifying the code a lot, but I'd
rather leave it in case someone wants to go back to implementing the
lazy stack pointer saving optimization.
In particular, I don't know precisely what the rules are for combining
alloca() and VLAs. I don't think you're allowed to do so in the same
scope, but I'm not at all confident our code works if we do both in
the same function.
That said, our VLA code can be made a lot simpler (simple enough for
even me to understand it) by giving up on the optimization idea, which
is very tempting. There's a patch to do that attached, feel free to
test and commit it if you like. (It passes all the tests, at least).
>
> _______________________________________________
> Tinycc-devel mailing list
> [email protected]
> https://lists.nongnu.org/mailman/listinfo/tinycc-devel
diff --git a/tcc.h b/tcc.h
index 384e8de..f63d83c 100644
--- a/tcc.h
+++ b/tcc.h
@@ -464,12 +464,6 @@ typedef struct DLLReference {
#define SYM_FIELD 0x20000000 /* struct/union field symbol space */
#define SYM_FIRST_ANOM 0x10000000 /* first anonymous sym */
-#define VLA_SP_LOC_SET 0x01 /* Location of SP on stack has been allocated
*/
-#define VLA_SP_SAVED 0x02 /* SP has been saved to slot already */
-#define VLA_NEED_NEW_FRAME 0x04 /* Needs new frame for next VLA */
-#define VLA_IN_SCOPE 0x08 /* One or more VLAs are in scope */
-#define VLA_SCOPE_FLAGS (VLA_SP_SAVED|VLA_NEED_NEW_FRAME|VLA_IN_SCOPE) /*
Flags which are saved and restored upon entering and exiting a block */
-
/* stored in 'Sym.c' field */
#define FUNC_NEW 1 /* ansi function prototype */
#define FUNC_OLD 2 /* old function prototype */
diff --git a/tccgen.c b/tccgen.c
index 8c1165f..9df285b 100644
--- a/tccgen.c
+++ b/tccgen.c
@@ -55,10 +55,9 @@ ST_DATA Sym *define_stack;
ST_DATA Sym *global_label_stack;
ST_DATA Sym *local_label_stack;
-ST_DATA int vla_sp_loc_tmp; /* vla_sp_loc is set to this when the value won't
be needed later */
+ST_DATA int vlas_in_scope; /* number of VLAs that are currently in scope */
ST_DATA int vla_sp_root_loc; /* vla_sp_loc for SP before any VLAs were pushed
*/
-ST_DATA int *vla_sp_loc; /* Pointer to variable holding location to store
stack pointer on the stack when modifying stack pointer */
-ST_DATA int vla_flags; /* VLA_* flags */
+ST_DATA int vla_sp_loc; /* Pointer to variable holding location to store stack
pointer on the stack when modifying stack pointer */
ST_DATA SValue __vstack[1+VSTACK_SIZE], *vtop;
@@ -87,7 +86,8 @@ static int decl0(int l, int is_for_loop_init);
static void expr_eq(void);
static void unary_type(CType *type);
static void vla_runtime_type_size(CType *type, int *a);
-static void vla_sp_save(void);
+static void vla_sp_restore(void);
+static void vla_sp_restore_root(void);
static int is_compatible_parameter_types(CType *type1, CType *type2);
static void expr_type(CType *type);
ST_FUNC void vpush64(int ty, unsigned long long v);
@@ -2223,14 +2223,15 @@ ST_FUNC void vla_runtime_type_size(CType *type, int *a)
}
}
-static void vla_sp_save(void) {
- if (!(vla_flags & VLA_SP_LOC_SET)) {
- *vla_sp_loc = (loc -= PTR_SIZE);
- vla_flags |= VLA_SP_LOC_SET;
+static void vla_sp_restore(void) {
+ if (vlas_in_scope) {
+ gen_vla_sp_restore(vla_sp_loc);
}
- if (!(vla_flags & VLA_SP_SAVED)) {
- gen_vla_sp_save(*vla_sp_loc);
- vla_flags |= VLA_SP_SAVED;
+}
+
+static void vla_sp_restore_root(void) {
+ if (vlas_in_scope) {
+ gen_vla_sp_restore(vla_sp_root_loc);
}
}
@@ -4794,10 +4795,7 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
} else if (tok == TOK_WHILE) {
next();
d = ind;
- if (vla_flags & VLA_IN_SCOPE) {
- gen_vla_sp_restore(*vla_sp_loc);
- vla_flags |= VLA_NEED_NEW_FRAME;
- }
+ vla_sp_restore();
skip('(');
gexpr();
skip(')');
@@ -4809,7 +4807,7 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
gsym_addr(b, d);
} else if (tok == '{') {
Sym *llabel;
- int block_vla_sp_loc, *saved_vla_sp_loc, saved_vla_flags,
*orig_vla_sp_loc;
+ int block_vla_sp_loc = vla_sp_loc, saved_vlas_in_scope = vlas_in_scope;
next();
/* record local declaration stack position */
@@ -4819,15 +4817,6 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
scope_stack_bottom = frame_bottom;
llabel = local_label_stack;
- /* save VLA state */
- block_vla_sp_loc = *(saved_vla_sp_loc = vla_sp_loc);
- if (saved_vla_sp_loc != &vla_sp_root_loc)
- vla_sp_loc = &block_vla_sp_loc;
- orig_vla_sp_loc = vla_sp_loc;
-
- saved_vla_flags = vla_flags;
- vla_flags |= VLA_NEED_NEW_FRAME;
-
/* handle local labels declarations */
if (tok == TOK_LABEL) {
next();
@@ -4877,13 +4866,11 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
sym_pop(&local_stack, s);
/* Pop VLA frames and restore stack pointer if required */
- if (saved_vla_sp_loc != &vla_sp_root_loc)
- *saved_vla_sp_loc = block_vla_sp_loc;
- if (vla_sp_loc != orig_vla_sp_loc) {
- gen_vla_sp_restore(*saved_vla_sp_loc);
+ if (vlas_in_scope > saved_vlas_in_scope) {
+ vla_sp_loc = saved_vlas_in_scope ? block_vla_sp_loc :
vla_sp_root_loc;
+ vla_sp_restore();
}
- vla_sp_loc = saved_vla_sp_loc;
- vla_flags = (vla_flags & ~VLA_SCOPE_FLAGS) | (saved_vla_flags &
VLA_SCOPE_FLAGS);
+ vlas_in_scope = saved_vlas_in_scope;
next();
} else if (tok == TOK_RETURN) {
@@ -4963,13 +4950,7 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
/* compute jump */
if (!csym)
tcc_error("cannot continue");
- if (vla_flags & VLA_IN_SCOPE) {
- /* If VLAs are in use, save the current stack pointer and
- reset the stack pointer to what it was at function entry
- (label will restore stack pointer in inner scopes) */
- vla_sp_save();
- gen_vla_sp_restore(vla_sp_root_loc);
- }
+ vla_sp_restore_root();
*csym = gjmp(*csym);
next();
skip(';');
@@ -4992,10 +4973,7 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
skip(';');
d = ind;
c = ind;
- if (vla_flags & VLA_IN_SCOPE) {
- gen_vla_sp_restore(*vla_sp_loc);
- vla_flags |= VLA_NEED_NEW_FRAME;
- }
+ vla_sp_restore();
a = 0;
b = 0;
if (tok != ';') {
@@ -5006,10 +4984,7 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
if (tok != ')') {
e = gjmp(0);
c = ind;
- if (vla_flags & VLA_IN_SCOPE) {
- gen_vla_sp_restore(*vla_sp_loc);
- vla_flags |= VLA_NEED_NEW_FRAME;
- }
+ vla_sp_restore();
gexpr();
vpop();
gjmp_addr(d);
@@ -5028,10 +5003,7 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
a = 0;
b = 0;
d = ind;
- if (vla_flags & VLA_IN_SCOPE) {
- gen_vla_sp_restore(*vla_sp_loc);
- vla_flags |= VLA_NEED_NEW_FRAME;
- }
+ vla_sp_restore();
block(&a, &b, case_sym, def_sym, case_reg, 0);
skip(TOK_WHILE);
skip('(');
@@ -5129,14 +5101,7 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
if (s->r == LABEL_DECLARED)
s->r = LABEL_FORWARD;
}
- /* label already defined */
- if (vla_flags & VLA_IN_SCOPE) {
- /* If VLAs are in use, save the current stack pointer and
- reset the stack pointer to what it was at function entry
- (label will restore stack pointer in inner scopes) */
- vla_sp_save();
- gen_vla_sp_restore(vla_sp_root_loc);
- }
+ vla_sp_restore_root();
if (s->r & LABEL_FORWARD)
s->jnext = gjmp(s->jnext);
else
@@ -5152,12 +5117,6 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
b = is_label();
if (b) {
/* label case */
- if (vla_flags & VLA_IN_SCOPE) {
- /* save/restore stack pointer across label
- this is a no-op when combined with the load immediately
- after the label unless we arrive via goto */
- vla_sp_save();
- }
s = label_find(b);
if (s) {
if (s->r == LABEL_DEFINED)
@@ -5168,10 +5127,7 @@ static void block(int *bsym, int *csym, int *case_sym,
int *def_sym,
s = label_push(&global_label_stack, b, LABEL_DEFINED);
}
s->jnext = ind;
- if (vla_flags & VLA_IN_SCOPE) {
- gen_vla_sp_restore(*vla_sp_loc);
- vla_flags |= VLA_NEED_NEW_FRAME;
- }
+ vla_sp_restore();
/* we accept this, but it is a mistake */
block_after_label:
if (tok == '}') {
@@ -5475,20 +5431,17 @@ static void decl_initializer(CType *type, Section *sec,
unsigned long c,
int a;
/* save current stack pointer */
- if (vla_flags & VLA_NEED_NEW_FRAME) {
- vla_sp_save();
- vla_flags = VLA_IN_SCOPE;
- vla_sp_loc = &vla_sp_loc_tmp;
+ if (vlas_in_scope == 0) {
+ if (vla_sp_root_loc == -1)
+ vla_sp_root_loc = (loc -= PTR_SIZE);
+ gen_vla_sp_save(vla_sp_root_loc);
}
vla_runtime_type_size(type, &a);
gen_vla_alloc(type, a);
- vla_flags = VLA_IN_SCOPE;
- vla_sp_save();
- vset(type, VT_LOCAL|VT_LVAL, c);
- vswap();
- vstore();
- vpop();
+ gen_vla_sp_save(c);
+ vla_sp_loc = c;
+ vlas_in_scope++;
} else if (type->t & VT_ARRAY) {
s = type->ref;
n = s->c;
@@ -6071,8 +6024,8 @@ static void gen_function(Sym *sym)
funcname = get_tok_str(sym->v, NULL);
func_ind = ind;
/* Initialize VLA state */
- vla_sp_loc = &vla_sp_root_loc;
- vla_flags = VLA_NEED_NEW_FRAME;
+ vla_sp_loc = -1;
+ vla_sp_root_loc = -1;
/* put debug symbol */
if (tcc_state->do_debug)
put_func_debug(sym);
diff --git a/x86_64-gen.c b/x86_64-gen.c
index c7a35d8..5a080af 100644
--- a/x86_64-gen.c
+++ b/x86_64-gen.c
@@ -2240,11 +2240,7 @@ ST_FUNC void gen_vla_alloc(CType *type, int align) {
/* We align to 16 bytes rather than align */
/* and ~15, %rsp */
o(0xf0e48348);
- /* mov %rsp, r */
- o(0x8948);
- o(0xe0 | REG_VALUE(r));
vpop();
- vset(type, r, 0);
#endif
}
_______________________________________________
Tinycc-devel mailing list
[email protected]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
