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