From f26259caac62b30bc710f6aa28c6e08253b6c5b2 Mon Sep 17 00:00:00 2001 From: George Koehler Date: Fri, 2 Mar 2018 16:06:21 -0500 Subject: [PATCH] Check AAR earlier to prevent LOI/STI unknown size. In ego, the CS phase may convert a LAR/SAR to AAR LOI/STI so it can optimize multiple occurrences of AAR of the same array element. This conversion should not happen if it would LOI/STI a large or unknown size. cs_profit.c okay_lines() checked the size of each occurrence of AAR except the first. If the first AAR was the implicit AAR in a LAR/SAR, then the conversion happened without checking the size. For unknown size, this made a bad LOI -1 or STI -1. Fix by checking the size earlier: if a LAR/SAR has a bad size, then don't enter it as an AAR. This Modula-2 code showed the bug. Given M.def: DEFINITION MODULE M; TYPE S = SET OF [0..95]; PROCEDURE F(a: ARRAY OF S; i, j: INTEGER); END M. and M.mod: (*$R-*) IMPLEMENTATION MODULE M; FROM SYSTEM IMPORT ADDRESS, ADR; PROCEDURE G(s: S; p, q: ADDRESS; t: S); BEGIN s := s; p := p; q := q; t := t; END G; PROCEDURE F(a: ARRAY OF S; i, j: INTEGER); BEGIN G(a[i + j], ADR(a[i + j]), ADR(a[i + j]), a[i + j]) END F; END M. then the bug caused an error: $ ack -mlinuxppc -O3 -c.e M.mod /tmp/Ack_b357d.g, line 57: Argument range error The bug had put LOI -1 in the code, then em_decode got an error because -1 is out of range for LOI. Procedure F has 4 occurrences of `a[i + j]`. The size of `a[i + j]` is 96 bits, or 12 bytes, but the EM code hides the size in an array descriptor, so the size is unknown to CS. The pragma `(*$R-*)` disables a range check on `i + j` so CS can work. EM uses AAR for the 2 `ADR(a[i + j])` and LAR for the other 2 `a[i + j]`. EM pushes the arguments to G in reverse order, so the last `a[i + j]` in Modula-2 is the first LAR in EM. CS found 4 occurrences of AAR. The first AAR was an implicit AAR in LAR. Because of the bug, CS converted this LAR 4 to AAR 4 LOI -1. --- util/ego/cs/cs_profit.c | 31 ++++++++++++++++++------------- util/ego/cs/cs_profit.h | 6 ++++++ util/ego/cs/cs_vnm.c | 17 ++++++++++++----- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/util/ego/cs/cs_profit.c b/util/ego/cs/cs_profit.c index 50cb708fd..8845aaa29 100644 --- a/util/ego/cs/cs_profit.c +++ b/util/ego/cs/cs_profit.c @@ -111,6 +111,21 @@ void cs_machinit(void *vp) choose_cset(f, &forbidden, sp_lmnem); } +bool may_become_aar(avail_p avp) +{ + /* Check whether it is desirable to treat a LAR or SAR as an + * AAR LOI/STI. This depends on the size of the array-elements. + */ + offset sz; + + sz = array_elemsize(avp->av_othird); + if (sz == UNKNOWN_SIZE) + return FALSE; + if (time_space_ratio < 50) + return sz <= AR_limit; + return TRUE; +} + STATIC bool sli_no_eliminate(line_p lnp) { /* Return whether the SLI-instruction in lnp is part of @@ -157,8 +172,10 @@ STATIC bool gains(avail_p avp) STATIC bool okay_lines(avail_p avp, occur_p ocp) { + /* Check whether all lines in this occurrence can in + * principle be eliminated; no stores, messages, calls etc. + */ register line_p lnp, next; - offset sz; for (lnp = ocp->oc_lfirst; lnp != (line_p) 0; lnp = next) { next = lnp != ocp->oc_llast ? lnp->l_next : (line_p) 0; @@ -171,18 +188,6 @@ STATIC bool okay_lines(avail_p avp, occur_p ocp) return FALSE; } } - /* All lines in this occurrence can in principle be eliminated; - * no stores, messages, calls etc. - * We now check whether it is desirable to treat a LAR or a SAR - * as an AAR LOI/STI. This depends on the size of the array-elements. - */ - if (INSTR(ocp->oc_llast) == op_lar || INSTR(ocp->oc_llast) == op_sar) { - sz = array_elemsize(avp->av_othird); - if (sz == UNKNOWN_SIZE) return FALSE; - if (avp->av_instr == (byte) op_aar && time_space_ratio < 50) { - return sz <= AR_limit; - } - } return TRUE; } diff --git a/util/ego/cs/cs_profit.h b/util/ego/cs/cs_profit.h index 7ec5e3c17..43f2bade9 100644 --- a/util/ego/cs/cs_profit.h +++ b/util/ego/cs/cs_profit.h @@ -7,6 +7,12 @@ void cs_machinit(void *vp); /* (FILE *f) * Read phase-specific information from f. */ +bool may_become_aar(avail_p avp); + /* + * Return whether a LAR/SAR may become + * an AAR LOI/STI. + */ + bool desirable(avail_p avp); /* * Return whether it is desirable to eliminate * the recurrences of the expression in avp. diff --git a/util/ego/cs/cs_vnm.c b/util/ego/cs/cs_vnm.c index 4dbeb3df2..67507f805 100644 --- a/util/ego/cs/cs_vnm.c +++ b/util/ego/cs/cs_vnm.c @@ -50,11 +50,13 @@ STATIC void put_expensive_load(bblock_p bp, line_p lnp, line_p lfirst, STATIC void put_aar(bblock_p bp, line_p lnp, line_p lfirst, entity_p enp) { - /* Enp points to an ENARRELEM. We do as if its address was computed. */ - + /* Enter the implicit AAR in a LAR or SAR, where enp points to + * the ENARRELEM, and AAR computes its address. + */ struct avail av; occur_p ocp; + assert(INSTR(lnp) == op_lar || INSTR(lnp) == op_sar); assert(enp->en_kind == ENARRELEM); av.av_instr = op_aar; av.av_size = ps; @@ -62,9 +64,14 @@ STATIC void put_aar(bblock_p bp, line_p lnp, line_p lfirst, entity_p enp) av.av_osecond = enp->en_index; av.av_othird = enp->en_adesc; - ocp = newoccur(lfirst, lnp, bp); - - av_enter(&av, ocp, TERNAIR_OP); + /* Before we enter an available AAR, we must check whether we + * may convert this LAR/SAR to AAR LOI/STI. This is so we + * don't LOI/STI a large or unknown size. + */ + if (may_become_aar(&av)) { + ocp = newoccur(lfirst, lnp, bp); + av_enter(&av, ocp, TERNAIR_OP); + } } STATIC void push_avail(avail_p avp, line_p lfirst)