1 | ----------------------- REVIEW 1 --------------------- |
---|
2 | PAPER: 9 |
---|
3 | TITLE: On the correctness of a branch displacement algorithm |
---|
4 | AUTHORS: Jaap Boender and Claudio Sacerdoti Coen |
---|
5 | |
---|
6 | REVIEWER'S CONFIDENCE: 4 (expert) |
---|
7 | |
---|
8 | Matita verification of an algorithm for branch displacement, which is |
---|
9 | compilation of assembly language jump instructions into concrete machine |
---|
10 | language jump instructions of various sizes and performance characteristics and |
---|
11 | with varying addressing modes, aiming to optimize performance. Execution is by |
---|
12 | a fixed-point iteration, repeatedly promoting short-range jumps into |
---|
13 | longer-range jumps as needed. |
---|
14 | |
---|
15 | This seems to be the first treatment of an important part of a full verified |
---|
16 | compiler pipeline, so I recommend acceptance. The prose is good, but I found |
---|
17 | the Matita code examples to be outright punitive for the reader (or at least |
---|
18 | for this reader). Anyone wanting that much detail will read your |
---|
19 | implementation; you can use a conference paper more effectively by sticking to |
---|
20 | the big picture, favoring diagrams and conventional mathematical notation. I |
---|
21 | expect most of the code details would really be close to obvious to a proof |
---|
22 | assistant expert who had read a good informal description of your approach. |
---|
23 | |
---|
24 | So, I would recommend removing almost all of that part of the paper, which |
---|
25 | opens up space that could be used to merge with the concurrent CPP submission |
---|
26 | on a different aspect of the same project. The introduction and related work |
---|
27 | sections of the two papers are already almost the same. |
---|
28 | |
---|
29 | p4: "the distance between L0 and the branch to it are too large": "are" ought |
---|
30 | o be "is". |
---|
31 | p9: "exeception" -> "exception" |
---|
32 | p10: Problem with underscores being interpreted by LaTeX as subscripts in |
---|
33 | "ppc_pc_map" |
---|
34 | |
---|
35 | ----------------------- REVIEW 2 --------------------- |
---|
36 | PAPER: 9 |
---|
37 | TITLE: On the correctness of a branch displacement algorithm |
---|
38 | AUTHORS: Jaap Boender and Claudio Sacerdoti Coen |
---|
39 | |
---|
40 | REVIEWER'S CONFIDENCE: 3 (high) |
---|
41 | |
---|
42 | This paper discusses the formal verification of an important assembler-level |
---|
43 | code-size optimisation for a simple, resource constrained processor: deciding |
---|
44 | which branch instruction to use for each conditional and unconditional jump (the |
---|
45 | branch instructions vary in size). The algorithm and the informal reasons why |
---|
46 | it is correct are well explained. However, the paper, as presented, is not |
---|
47 | ready for publication; it sketches the proof, but does not draw out general |
---|
48 | ideas that might be of use to the reader. Because the algorithm verified is not |
---|
49 | particularly complicated, this kind of contribution is essential if the paper to |
---|
50 | stand on its own. |
---|
51 | |
---|
52 | Here are two possibilities that would strengthen the paper and address the above |
---|
53 | criticism: |
---|
54 | |
---|
55 | - an analysis of the trade-offs made in the design of the algorithm in order to |
---|
56 | support verification. Quantifying how much, in practice, the choices about |
---|
57 | always lengthening branches (which lead to an easy termination proof) increase |
---|
58 | the code size would give a good idea of whether its worth trying to invest the |
---|
59 | effort to verify a more complex version in the future. |
---|
60 | |
---|
61 | - motivation for the main statement of correctness, by showing how it fits in to |
---|
62 | the overall verification of the assembler. I can see how the main theorem on |
---|
63 | page 9 is a necessary condition for the algorithm to be correct, but it is not |
---|
64 | clear that it is sufficient, or how it would be used in the overall |
---|
65 | correctness statement of an assembler. It constrains sigma (which is what the |
---|
66 | verified algorithm calculates), but the paper does not formalise the use of |
---|
67 | sigma to actually transform the code. Thus, it is unclear that the |
---|
68 | correctness statement is really saying everything it needs to. In particular, |
---|
69 | the semantics of the assembly language or machine code are not mentioned in |
---|
70 | it, even though the correctness of the assembler must be stated in term of |
---|
71 | them. |
---|
72 | |
---|
73 | ----------------------- REVIEW 3 --------------------- |
---|
74 | PAPER: 9 |
---|
75 | TITLE: On the correctness of a branch displacement algorithm |
---|
76 | AUTHORS: Jaap Boender and Claudio Sacerdoti Coen |
---|
77 | |
---|
78 | REVIEWER'S CONFIDENCE: 3 (high) |
---|
79 | |
---|
80 | This paper presents a new branch displacement algorithm, and certified |
---|
81 | its correctness in the Matita proof assistant. |
---|
82 | |
---|
83 | I feel the technical contribution of the work is a bit too thin. |
---|
84 | For the algorithm design, if the authors view it as a contribution, |
---|
85 | there should be more justification of it. The authors mention |
---|
86 | the existence of sub-optimal algorithms in SDCC and gcc. Is the new |
---|
87 | algorithm better than them? The authors mention that the new algorithm |
---|
88 | is "at least as good as the ones from SDCC and gcc, and potentially |
---|
89 | better. Its complexity remains the same ..." Can the authors provide |
---|
90 | data to justify the claim? Also such a claim does not sound impressive |
---|
91 | at all. |
---|
92 | |
---|
93 | For the verification, it is unclear to me why it is challenging. |
---|
94 | It seems the correctness is defined based on syntactic comparison |
---|
95 | between the assembly code and the target machine code only, which |
---|
96 | should be straightforward. It does not involve semantics preservation |
---|
97 | and simulation proof. |
---|
98 | |
---|
99 | Also, instead of verifying the algorithm directly, would it be simpler |
---|
100 | to implement a validator to check the consistency between the input |
---|
101 | and the output of the algorithm, and then verify the correctness of the |
---|
102 | validator? |
---|
103 | |
---|
104 | ----------------------- REVIEW 4 --------------------- |
---|
105 | PAPER: 9 |
---|
106 | TITLE: On the correctness of a branch displacement algorithm |
---|
107 | AUTHORS: Jaap Boender and Claudio Sacerdoti Coen |
---|
108 | |
---|
109 | REVIEWER'S CONFIDENCE: 4 (expert) |
---|
110 | |
---|
111 | This paper verifies an algorithm for choosing branch instructions in an |
---|
112 | assembler. The algorithm requires multiple iterations, since a choice for one |
---|
113 | branch instruction can affect the available choices for other branch |
---|
114 | instructions. |
---|
115 | |
---|
116 | I found it hard to consider this paper separately from the CPP submission |
---|
117 | number 16 ("On the correctness of an optimising assembler for the MCS-51 |
---|
118 | microcontroller"). The "main correctness statement", for example, isn't so |
---|
119 | interesting in isolation; what's more interesting is that the correctness of |
---|
120 | the branch displacement algorithm is a building block for the correctness of |
---|
121 | the entire assembler. Both papers contain text introducing the same issues |
---|
122 | regarding branch displacement, although this paper is the only one that |
---|
123 | presents the actual algorithm for solving the branch displacement problem. |
---|
124 | The algorithm itself is short enough to fit in either paper; it's the |
---|
125 | exhaustive list of detailed invariants that require a second paper. However, |
---|
126 | these invariants aren't particularly easy or interesting to read. |
---|
127 | |
---|
128 | One problem with the invariants' readability is the disconnection between the |
---|
129 | algorithm's notation (in Figure 5) and the invariants' notation. The variable |
---|
130 | names are often different, and types are sometimes different ("acc" is a triple |
---|
131 | in Figure 5, but is described as a pair in section 4.1). To improve the |
---|
132 | readability, I'd suggest annotating Figure 5 with Floyd-Hoare style invariants |
---|
133 | (e.g. putting something like "invariant sigma_compact_unsafe(..., sigma)" |
---|
134 | directly in the code). To reduce the bulkiness of the invariants, you might |
---|
135 | also consider using mathematical notation rather than Matita text, and only |
---|
136 | showing a few small examples of Matita text to give the reader a sense of what |
---|
137 | they look like. |
---|
138 | |
---|
139 | Two cited papers (Robertson[8] and Szymanski[11]) show the NP-completeness of |
---|
140 | problems closely related to the MCS-51 branch displacement problem. However, |
---|
141 | it's not obvious that the results imply the NP-completeness for MCS-51. Each |
---|
142 | cited paper shows a polynomial-time solution on a simple problem followed by an |
---|
143 | NP-completeness proof for an extended problem. For Robertson[8], the extension |
---|
144 | has to do with storage allocation, which seems different from the MCS-51 |
---|
145 | problem. For Szymanski[11], the extension has to do with complex assembly-level |
---|
146 | expressions based on adding and subtracting label addresses. The MCS-51 |
---|
147 | assembler doesn't seem to have this feature, so it's not clear that the |
---|
148 | NP-completeness result can be taken directly from Szymanski[11], even if both |
---|
149 | settings deal with similar "pathological" jumps. If you want to rigorously |
---|
150 | claim NP-completeness for your particular problem, you probably have to do your |
---|
151 | own reduction. Alternately, you can just say that similar problems are known |
---|
152 | to be NP-complete, without claiming NP-completeness for the MCS-51 problem. |
---|
153 | |
---|
154 | minor comments: |
---|
155 | |
---|
156 | Figure 1 lists sizes of x86-64 instructions. The last two entries appear to be |
---|
157 | 64-bit direct jumps, which surely would require more than 8 bytes to encode. |
---|
158 | Perhaps these are indirect jumps? |
---|
159 | |
---|
160 | Figure 5 doesn't initialize the variable "new_sigma". |
---|
161 | |
---|
162 | Section 4.1: "ppc_pc_map" is misformatted in the third paragraph. |
---|