xref: /aosp_15_r20/external/llvm/docs/HistoricalNotes/2001-02-09-AdveCommentsResponse.txt (revision 9880d6810fe72a1726cb53787c6711e909410d58)
1*9880d681SAndroid Build Coastguard WorkerFrom: Chris Lattner <[email protected]>
2*9880d681SAndroid Build Coastguard WorkerTo: "Vikram S. Adve" <[email protected]>
3*9880d681SAndroid Build Coastguard WorkerSubject: Re: LLVM Feedback
4*9880d681SAndroid Build Coastguard Worker
5*9880d681SAndroid Build Coastguard WorkerI've included your feedback in the /home/vadve/lattner/llvm/docs directory
6*9880d681SAndroid Build Coastguard Workerso that it will live in CVS eventually with the rest of LLVM.  I've
7*9880d681SAndroid Build Coastguard Workersignificantly updated the documentation to reflect the changes you
8*9880d681SAndroid Build Coastguard Workersuggested, as specified below:
9*9880d681SAndroid Build Coastguard Worker
10*9880d681SAndroid Build Coastguard Worker> We should consider eliminating the type annotation in cases where it is
11*9880d681SAndroid Build Coastguard Worker> essentially obvious from the instruction type:
12*9880d681SAndroid Build Coastguard Worker>        br bool <cond>, label <iftrue>, label <iffalse>
13*9880d681SAndroid Build Coastguard Worker> I think your point was that making all types explicit improves clarity
14*9880d681SAndroid Build Coastguard Worker> and readability.  I agree to some extent, but it also comes at the
15*9880d681SAndroid Build Coastguard Worker> cost of verbosity.  And when the types are obvious from people's
16*9880d681SAndroid Build Coastguard Worker> experience (e.g., in the br instruction), it doesn't seem to help as
17*9880d681SAndroid Build Coastguard Worker> much.
18*9880d681SAndroid Build Coastguard Worker
19*9880d681SAndroid Build Coastguard WorkerVery true.  We should discuss this more, but my reasoning is more of a
20*9880d681SAndroid Build Coastguard Workerconsistency argument.  There are VERY few instructions that can have all
21*9880d681SAndroid Build Coastguard Workerof the types eliminated, and doing so when available unnecessarily makes
22*9880d681SAndroid Build Coastguard Workerthe language more difficult to handle.  Especially when you see 'int
23*9880d681SAndroid Build Coastguard Worker%this' and 'bool %that' all over the place, I think it would be
24*9880d681SAndroid Build Coastguard Workerdisorienting to see:
25*9880d681SAndroid Build Coastguard Worker
26*9880d681SAndroid Build Coastguard Worker  br %predicate, %iftrue, %iffalse
27*9880d681SAndroid Build Coastguard Worker
28*9880d681SAndroid Build Coastguard Workerfor branches.  Even just typing that once gives me the creeps. ;)  Like I
29*9880d681SAndroid Build Coastguard Workersaid, we should probably discuss this further in person...
30*9880d681SAndroid Build Coastguard Worker
31*9880d681SAndroid Build Coastguard Worker> On reflection, I really like your idea of having the two different
32*9880d681SAndroid Build Coastguard Worker> switch types (even though they encode implementation techniques rather
33*9880d681SAndroid Build Coastguard Worker> than semantics).  It should simplify building the CFG and my guess is it
34*9880d681SAndroid Build Coastguard Worker> could enable some significant optimizations, though we should think
35*9880d681SAndroid Build Coastguard Worker> about which.
36*9880d681SAndroid Build Coastguard Worker
37*9880d681SAndroid Build Coastguard WorkerGreat.  I added a note to the switch section commenting on how the VM
38*9880d681SAndroid Build Coastguard Workershould just use the instruction type as a hint, and that the
39*9880d681SAndroid Build Coastguard Workerimplementation may choose altermate representations (such as predicated
40*9880d681SAndroid Build Coastguard Workerbranches).
41*9880d681SAndroid Build Coastguard Worker
42*9880d681SAndroid Build Coastguard Worker> In the lookup-indirect form of the switch, is there a reason not to
43*9880d681SAndroid Build Coastguard Worker> make the val-type uint?
44*9880d681SAndroid Build Coastguard Worker
45*9880d681SAndroid Build Coastguard WorkerNo.  This was something I was debating for a while, and didn't really feel
46*9880d681SAndroid Build Coastguard Workerstrongly about either way.  It is common to switch on other types in HLL's
47*9880d681SAndroid Build Coastguard Worker(for example signed int's are particularly common), but in this case, all
48*9880d681SAndroid Build Coastguard Workerthat will be added is an additional 'cast' instruction.  I removed that
49*9880d681SAndroid Build Coastguard Workerfrom the spec.
50*9880d681SAndroid Build Coastguard Worker
51*9880d681SAndroid Build Coastguard Worker> I agree with your comment that we don't need 'neg'
52*9880d681SAndroid Build Coastguard Worker
53*9880d681SAndroid Build Coastguard WorkerRemoved.
54*9880d681SAndroid Build Coastguard Worker
55*9880d681SAndroid Build Coastguard Worker> There's a trade-off with the cast instruction:
56*9880d681SAndroid Build Coastguard Worker>  +  it avoids having to define all the upcasts and downcasts that are
57*9880d681SAndroid Build Coastguard Worker>     valid for the operands of each instruction  (you probably have
58*9880d681SAndroid Build Coastguard Worker>     thought of other benefits also)
59*9880d681SAndroid Build Coastguard Worker>  -  it could make the bytecode significantly larger because there could
60*9880d681SAndroid Build Coastguard Worker>     be a lot of cast operations
61*9880d681SAndroid Build Coastguard Worker
62*9880d681SAndroid Build Coastguard Worker + You NEED casts to represent things like:
63*9880d681SAndroid Build Coastguard Worker    void foo(float);
64*9880d681SAndroid Build Coastguard Worker    ...
65*9880d681SAndroid Build Coastguard Worker    int x;
66*9880d681SAndroid Build Coastguard Worker    ...
67*9880d681SAndroid Build Coastguard Worker    foo(x);
68*9880d681SAndroid Build Coastguard Worker   in a language like C.  Even in a Java like language, you need upcasts
69*9880d681SAndroid Build Coastguard Worker   and some way to implement dynamic downcasts.
70*9880d681SAndroid Build Coastguard Worker + Not all forms of instructions take every type (for example you can't
71*9880d681SAndroid Build Coastguard Worker   shift by a floating point number of bits), thus SOME programs will need
72*9880d681SAndroid Build Coastguard Worker   implicit casts.
73*9880d681SAndroid Build Coastguard Worker
74*9880d681SAndroid Build Coastguard WorkerTo be efficient and to avoid your '-' point above, we just have to be
75*9880d681SAndroid Build Coastguard Workercareful to specify that the instructions shall operate on all common
76*9880d681SAndroid Build Coastguard Workertypes, therefore casting should be relatively uncommon.  For example all
77*9880d681SAndroid Build Coastguard Workerof the arithmetic operations work on almost all data types.
78*9880d681SAndroid Build Coastguard Worker
79*9880d681SAndroid Build Coastguard Worker> Making the second arg. to 'shl' a ubyte seems good enough to me.
80*9880d681SAndroid Build Coastguard Worker> 255 positions seems adequate for several generations of machines
81*9880d681SAndroid Build Coastguard Worker
82*9880d681SAndroid Build Coastguard WorkerOkay, that comment is removed.
83*9880d681SAndroid Build Coastguard Worker
84*9880d681SAndroid Build Coastguard Worker> and is more compact than uint.
85*9880d681SAndroid Build Coastguard Worker
86*9880d681SAndroid Build Coastguard WorkerNo, it isn't.  Remember that the bytecode encoding saves value slots into
87*9880d681SAndroid Build Coastguard Workerthe bytecode instructions themselves, not constant values.  This is
88*9880d681SAndroid Build Coastguard Workeranother case where we may introduce more cast instructions (but we will
89*9880d681SAndroid Build Coastguard Workeralso reduce the number of opcode variants that must be supported by a
90*9880d681SAndroid Build Coastguard Workervirtual machine).  Because most shifts are by constant values, I don't
91*9880d681SAndroid Build Coastguard Workerthink that we'll have to cast many shifts.  :)
92*9880d681SAndroid Build Coastguard Worker
93*9880d681SAndroid Build Coastguard Worker> I still have some major concerns about including malloc and free in the
94*9880d681SAndroid Build Coastguard Worker> language (either as builtin functions or instructions).
95*9880d681SAndroid Build Coastguard Worker
96*9880d681SAndroid Build Coastguard WorkerAgreed.  How about this proposal:
97*9880d681SAndroid Build Coastguard Worker
98*9880d681SAndroid Build Coastguard Workermalloc/free are either built in functions or actual opcodes.  They provide
99*9880d681SAndroid Build Coastguard Workerall of the type safety that the document would indicate, blah blah
100*9880d681SAndroid Build Coastguard Workerblah. :)
101*9880d681SAndroid Build Coastguard Worker
102*9880d681SAndroid Build Coastguard WorkerNow, because of all of the excellent points that you raised, an
103*9880d681SAndroid Build Coastguard Workerimplementation may want to override the default malloc/free behavior of
104*9880d681SAndroid Build Coastguard Workerthe program.  To do this, they simply implement a "malloc" and
105*9880d681SAndroid Build Coastguard Worker"free" function.  The virtual machine will then be defined to use the user
106*9880d681SAndroid Build Coastguard Workerdefined malloc/free function (which return/take void*'s, not type'd
107*9880d681SAndroid Build Coastguard Workerpointers like the builtin function would) if one is available, otherwise
108*9880d681SAndroid Build Coastguard Workerfall back on a system malloc/free.
109*9880d681SAndroid Build Coastguard Worker
110*9880d681SAndroid Build Coastguard WorkerDoes this sound like a good compromise?  It would give us all of the
111*9880d681SAndroid Build Coastguard Workertypesafety/elegance in the language while still allowing the user to do
112*9880d681SAndroid Build Coastguard Workerall the cool stuff they want to...
113*9880d681SAndroid Build Coastguard Worker
114*9880d681SAndroid Build Coastguard Worker>  'alloca' on the other hand sounds like a good idea, and the
115*9880d681SAndroid Build Coastguard Worker>  implementation seems fairly language-independent so it doesn't have the
116*9880d681SAndroid Build Coastguard Worker>  problems with malloc listed above.
117*9880d681SAndroid Build Coastguard Worker
118*9880d681SAndroid Build Coastguard WorkerOkay, once we get the above stuff figured out, I'll put it all in the
119*9880d681SAndroid Build Coastguard Workerspec.
120*9880d681SAndroid Build Coastguard Worker
121*9880d681SAndroid Build Coastguard Worker>  About indirect call:
122*9880d681SAndroid Build Coastguard Worker>  Your option #2 sounded good to me.  I'm not sure I understand your
123*9880d681SAndroid Build Coastguard Worker>  concern about an explicit 'icall' instruction?
124*9880d681SAndroid Build Coastguard Worker
125*9880d681SAndroid Build Coastguard WorkerI worry too much.  :)  The other alternative has been removed. 'icall' is
126*9880d681SAndroid Build Coastguard Workernow up in the instruction list next to 'call'.
127*9880d681SAndroid Build Coastguard Worker
128*9880d681SAndroid Build Coastguard Worker> I believe tail calls are relatively easy to identify; do you know why
129*9880d681SAndroid Build Coastguard Worker> .NET has a tailcall instruction?
130*9880d681SAndroid Build Coastguard Worker
131*9880d681SAndroid Build Coastguard WorkerAlthough I am just guessing, I believe it probably has to do with the fact
132*9880d681SAndroid Build Coastguard Workerthat they want languages like Haskell and lisp to be efficiently runnable
133*9880d681SAndroid Build Coastguard Workeron their VM.  Of course this means that the VM MUST implement tail calls
134*9880d681SAndroid Build Coastguard Worker'correctly', or else life will suck.  :)  I would put this into a future
135*9880d681SAndroid Build Coastguard Workerfeature bin, because it could be pretty handy...
136*9880d681SAndroid Build Coastguard Worker
137*9880d681SAndroid Build Coastguard Worker>  A pair of important synchronization instr'ns to think about:
138*9880d681SAndroid Build Coastguard Worker>    load-linked
139*9880d681SAndroid Build Coastguard Worker>    store-conditional
140*9880d681SAndroid Build Coastguard Worker
141*9880d681SAndroid Build Coastguard WorkerWhat is 'load-linked'?  I think that (at least for now) I should add these
142*9880d681SAndroid Build Coastguard Workerto the 'possible extensions' section, because they are not immediately
143*9880d681SAndroid Build Coastguard Workerneeded...
144*9880d681SAndroid Build Coastguard Worker
145*9880d681SAndroid Build Coastguard Worker> Other classes of instructions that are valuable for pipeline
146*9880d681SAndroid Build Coastguard Worker> performance:
147*9880d681SAndroid Build Coastguard Worker>    conditional-move
148*9880d681SAndroid Build Coastguard Worker>    predicated instructions
149*9880d681SAndroid Build Coastguard Worker
150*9880d681SAndroid Build Coastguard WorkerConditional move is effectly a special case of a predicated
151*9880d681SAndroid Build Coastguard Workerinstruction... and I think that all predicated instructions can possibly
152*9880d681SAndroid Build Coastguard Workerbe implemented later in LLVM.  It would significantly change things, and
153*9880d681SAndroid Build Coastguard Workerit doesn't seem to be very necessary right now.  It would seem to
154*9880d681SAndroid Build Coastguard Workercomplicate flow control analysis a LOT in the virtual machine.  I would
155*9880d681SAndroid Build Coastguard Workertend to prefer that a predicated architecture like IA64 convert from a
156*9880d681SAndroid Build Coastguard Worker"basic block" representation to a predicated rep as part of it's dynamic
157*9880d681SAndroid Build Coastguard Workercomplication phase.  Also, if a basic block contains ONLY a move, then
158*9880d681SAndroid Build Coastguard Workerthat can be trivally translated into a conditional move...
159*9880d681SAndroid Build Coastguard Worker
160*9880d681SAndroid Build Coastguard Worker> I agree that we need a static data space.  Otherwise, emulating global
161*9880d681SAndroid Build Coastguard Worker> data gets unnecessarily complex.
162*9880d681SAndroid Build Coastguard Worker
163*9880d681SAndroid Build Coastguard WorkerDefinitely.  Also a later item though.  :)
164*9880d681SAndroid Build Coastguard Worker
165*9880d681SAndroid Build Coastguard Worker> We once talked about adding a symbolic thread-id field to each
166*9880d681SAndroid Build Coastguard Worker> ..
167*9880d681SAndroid Build Coastguard Worker> Instead, it could a great topic for a separate study.
168*9880d681SAndroid Build Coastguard Worker
169*9880d681SAndroid Build Coastguard WorkerAgreed.  :)
170*9880d681SAndroid Build Coastguard Worker
171*9880d681SAndroid Build Coastguard Worker> What is the semantics of the IA64 stop bit?
172*9880d681SAndroid Build Coastguard Worker
173*9880d681SAndroid Build Coastguard WorkerBasically, the IA64 writes instructions like this:
174*9880d681SAndroid Build Coastguard Workermov ...
175*9880d681SAndroid Build Coastguard Workeradd ...
176*9880d681SAndroid Build Coastguard Workersub ...
177*9880d681SAndroid Build Coastguard Workerop xxx
178*9880d681SAndroid Build Coastguard Workerop xxx
179*9880d681SAndroid Build Coastguard Worker;;
180*9880d681SAndroid Build Coastguard Workermov ...
181*9880d681SAndroid Build Coastguard Workeradd ...
182*9880d681SAndroid Build Coastguard Workersub ...
183*9880d681SAndroid Build Coastguard Workerop xxx
184*9880d681SAndroid Build Coastguard Workerop xxx
185*9880d681SAndroid Build Coastguard Worker;;
186*9880d681SAndroid Build Coastguard Worker
187*9880d681SAndroid Build Coastguard WorkerWhere the ;; delimits a group of instruction with no dependencies between
188*9880d681SAndroid Build Coastguard Workerthem, which can all be executed concurrently (to the limits of the
189*9880d681SAndroid Build Coastguard Workeravailable functional units).  The ;; gets translated into a bit set in one
190*9880d681SAndroid Build Coastguard Workerof the opcodes.
191*9880d681SAndroid Build Coastguard Worker
192*9880d681SAndroid Build Coastguard WorkerThe advantages of this representation is that you don't have to do some
193*9880d681SAndroid Build Coastguard Workerkind of 'thread id scheduling' pass by having to specify ahead of time how
194*9880d681SAndroid Build Coastguard Workermany threads to use, and the representation doesn't have a per instruction
195*9880d681SAndroid Build Coastguard Workeroverhead...
196*9880d681SAndroid Build Coastguard Worker
197*9880d681SAndroid Build Coastguard Worker> And finally, another thought about the syntax for arrays :-)
198*9880d681SAndroid Build Coastguard Worker>  Although this syntax:
199*9880d681SAndroid Build Coastguard Worker>         array <dimension-list> of <type>
200*9880d681SAndroid Build Coastguard Worker>  is verbose, it will be used only in the human-readable assembly code so
201*9880d681SAndroid Build Coastguard Worker>  size should not matter.  I think we should consider it because I find it
202*9880d681SAndroid Build Coastguard Worker>  to be the clearest syntax.  It could even make arrays of function
203*9880d681SAndroid Build Coastguard Worker>  pointers somewhat readable.
204*9880d681SAndroid Build Coastguard Worker
205*9880d681SAndroid Build Coastguard WorkerMy only comment will be to give you an example of why this is a bad
206*9880d681SAndroid Build Coastguard Workeridea.  :)
207*9880d681SAndroid Build Coastguard Worker
208*9880d681SAndroid Build Coastguard WorkerHere is an example of using the switch statement (with my recommended
209*9880d681SAndroid Build Coastguard Workersyntax):
210*9880d681SAndroid Build Coastguard Worker
211*9880d681SAndroid Build Coastguard Workerswitch uint %val, label %otherwise,
212*9880d681SAndroid Build Coastguard Worker       [%3 x {uint, label}] [ { uint %57, label %l1 },
213*9880d681SAndroid Build Coastguard Worker                              { uint %20, label %l2 },
214*9880d681SAndroid Build Coastguard Worker                              { uint %14, label %l3 } ]
215*9880d681SAndroid Build Coastguard Worker
216*9880d681SAndroid Build Coastguard WorkerHere it is with the syntax you are proposing:
217*9880d681SAndroid Build Coastguard Worker
218*9880d681SAndroid Build Coastguard Workerswitch uint %val, label %otherwise,
219*9880d681SAndroid Build Coastguard Worker       array %3 of {uint, label}
220*9880d681SAndroid Build Coastguard Worker              array of {uint, label}
221*9880d681SAndroid Build Coastguard Worker                              { uint %57, label %l1 },
222*9880d681SAndroid Build Coastguard Worker                              { uint %20, label %l2 },
223*9880d681SAndroid Build Coastguard Worker                              { uint %14, label %l3 }
224*9880d681SAndroid Build Coastguard Worker
225*9880d681SAndroid Build Coastguard WorkerWhich is ambiguous and very verbose. It would be possible to specify
226*9880d681SAndroid Build Coastguard Workerconstants with [] brackets as in my syntax, which would look like this:
227*9880d681SAndroid Build Coastguard Worker
228*9880d681SAndroid Build Coastguard Workerswitch uint %val, label %otherwise,
229*9880d681SAndroid Build Coastguard Worker       array %3 of {uint, label}  [ { uint %57, label %l1 },
230*9880d681SAndroid Build Coastguard Worker                                    { uint %20, label %l2 },
231*9880d681SAndroid Build Coastguard Worker                                    { uint %14, label %l3 } ]
232*9880d681SAndroid Build Coastguard Worker
233*9880d681SAndroid Build Coastguard WorkerBut then the syntax is inconsistent between type definition and constant
234*9880d681SAndroid Build Coastguard Workerdefinition (why do []'s enclose the constants but not the types??).
235*9880d681SAndroid Build Coastguard Worker
236*9880d681SAndroid Build Coastguard WorkerAnyways, I'm sure that there is much debate still to be had over
237*9880d681SAndroid Build Coastguard Workerthis... :)
238*9880d681SAndroid Build Coastguard Worker
239*9880d681SAndroid Build Coastguard Worker-Chris
240*9880d681SAndroid Build Coastguard Worker
241*9880d681SAndroid Build Coastguard Workerhttp://www.nondot.org/~sabre/os/
242*9880d681SAndroid Build Coastguard Workerhttp://www.nondot.org/MagicStats/
243*9880d681SAndroid Build Coastguard Workerhttp://korbit.sourceforge.net/
244*9880d681SAndroid Build Coastguard Worker
245*9880d681SAndroid Build Coastguard Worker
246