On Tue, Feb 15, 2011 at 12:39 PM, Stéphane Ducasse < stephane.ducasse@inria.fr> wrote:
Eliot
I can understand that well. Now we could just let this code in VMMaker and not inlining. We fix 8 users and we are done. I have concerned that we have a complex solution because it is complex for a couple of use case. Of course we can do nothing (and we will probably not do it now) but in that case we can also stop pharo right now because a large part of the system could follow the exact same principle.
That's a false distinction. There's nothing wrong with caseOf: and not addressing it will not leave Pharo any the worse. Case statements are not in themselves evil in the right context. The main problems with the index/dictionary approach are that a) the solution is no longer contained in a single method b) the dictionary is metadata that needs to be updated in an initialize method; forget to initialize the dictionary when an index changes and your program is broken. As my first wife said "don't sweat the petty stuff; pet the sweaty stuff". BTW, there's a false assumption that has been stated in this thread that the caseOf: statement is only used with explicit integers. It can be used with anything. Expressions, class constants etc. BTW why marcus and jorge work on Opal because the compiler is just working
good?
Now I will focus on 1.2, FS, Opal if I can help, and the rest but caseOf: will be in our radar.
Just a remark if people would have spend the time to write mail to fix the users of caseof: in the image and other non VMmaker package we would have get already done.
Fix this (rhetorical - I think this is /much/ better as a single case statement than as an index/dictionary refactoring, both in Smalltalk and when translated to C): CogIA32Compiler>>computeMaximumSize "Compute the maximum size for each opcode. This allows jump offsets to be determined, provided that all backward branches are long branches." "N.B. The ^maxSize := N forms are to get around the compiler's long branch limits which are exceeded when each case jumps around the otherwise." opcode caseOf: { "Noops & Pseudo Ops" [Label] -> [^maxSize := 0]. [AlignmentNops] -> [^maxSize := (operands at: 0) - 1]. [Fill16] -> [^maxSize := 2]. [Fill32] -> [^maxSize := 4]. [FillFromWord] -> [^maxSize := 4]. [Nop] -> [^maxSize := 1]. "Specific Control/Data Movement" [CDQ] -> [^maxSize := 1]. [IDIVR] -> [^maxSize := 2]. [IMULRR] -> [^maxSize := 3]. [CPUID] -> [^maxSize := 2]. [CMPXCHGAwR] -> [^maxSize := 7]. [CMPXCHGMwrR] -> [^maxSize := 8]. [LFENCE] -> [^maxSize := 3]. [MFENCE] -> [^maxSize := 3]. [SFENCE] -> [^maxSize := 3]. [LOCK] -> [^maxSize := 1]. [XCHGAwR] -> [^maxSize := 6]. [XCHGMwrR] -> [^maxSize := 7]. [XCHGRR] -> [^maxSize := 2]. "Control" [Call] -> [^maxSize := 5]. [JumpR] -> [^maxSize := 2]. [Jump] -> [self resolveJumpTarget. ^maxSize := 5]. [JumpLong] -> [self resolveJumpTarget. ^maxSize := 5]. [JumpZero] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpNonZero] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpNegative] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpNonNegative] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpOverflow] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpNoOverflow] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpCarry] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpNoCarry] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpLess] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpGreaterOrEqual] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpGreater] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpLessOrEqual] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpBelow] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpAboveOrEqual] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpAbove] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpBelowOrEqual] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpLongZero] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpLongNonZero] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpFPEqual] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpFPNotEqual] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpFPLess] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpFPGreaterOrEqual] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpFPGreater] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpFPLessOrEqual] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpFPOrdered] -> [self resolveJumpTarget. ^maxSize := 6]. [JumpFPUnordered] -> [self resolveJumpTarget. ^maxSize := 6]. [RetN] -> [^maxSize := (operands at: 0) = 0 ifTrue: [1] ifFalse: [3]]. "Arithmetic" [AddCqR] -> [^maxSize := (self isQuick: (operands at: 0)) ifTrue: [3] ifFalse: [(self concreteRegister: (operands at: 1)) = EAX ifTrue: [5] ifFalse: [6]]]. [AndCqR] -> [^maxSize := (self isQuick: (operands at: 0)) ifTrue: [3] ifFalse: [(self concreteRegister: (operands at: 1)) = EAX ifTrue: [5] ifFalse: [6]]]. [CmpCqR] -> [^maxSize := (self isQuick: (operands at: 0)) ifTrue: [3] ifFalse: [(self concreteRegister: (operands at: 1)) = EAX ifTrue: [5] ifFalse: [6]]]. [OrCqR] -> [^maxSize := (self isQuick: (operands at: 0)) ifTrue: [3] ifFalse: [(self concreteRegister: (operands at: 1)) = EAX ifTrue: [5] ifFalse: [6]]]. [SubCqR] -> [^maxSize := (self isQuick: (operands at: 0)) ifTrue: [3] ifFalse: [(self concreteRegister: (operands at: 1)) = EAX ifTrue: [5] ifFalse: [6]]]. [AddCwR] -> [^maxSize := (self concreteRegister: (operands at: 1)) = EAX ifTrue: [5] ifFalse: [6]]. [CmpCwR] -> [^maxSize := (self concreteRegister: (operands at: 1)) = EAX ifTrue: [5] ifFalse: [6]]. [SubCwR] -> [^maxSize := (self concreteRegister: (operands at: 1)) = EAX ifTrue: [5] ifFalse: [6]]. [XorCwR] -> [^maxSize := (self concreteRegister: (operands at: 1)) = EAX ifTrue: [5] ifFalse: [6]]. [AddRR] -> [^maxSize := 2]. [AndRR] -> [^maxSize := 2]. [CmpRR] -> [^maxSize := 2]. [OrRR] -> [^maxSize := 2]. [XorRR] -> [^maxSize := 2]. [SubRR] -> [^maxSize := 2]. [NegateR] -> [^maxSize := 2]. [LoadEffectiveAddressMwrR] -> [^maxSize := ((self isQuick: (operands at: 0)) ifTrue: [3] ifFalse: [6]) + ((self concreteRegister: (operands at: 1)) = ESP ifTrue: [1] ifFalse: [0])]. [LogicalShiftLeftCqR] -> [^maxSize := (operands at: 0) = 1 ifTrue: [2] ifFalse: [3]]. [LogicalShiftRightCqR] -> [^maxSize := (operands at: 0) = 1 ifTrue: [2] ifFalse: [3]]. [ArithmeticShiftRightCqR] -> [^maxSize := (operands at: 0) = 1 ifTrue: [2] ifFalse: [3]]. [LogicalShiftLeftRR] -> [self computeShiftRRSize]. [LogicalShiftRightRR] -> [self computeShiftRRSize]. [ArithmeticShiftRightRR] -> [self computeShiftRRSize]. [AddRdRd] -> [^maxSize := 4]. [CmpRdRd] -> [^maxSize := 4]. [SubRdRd] -> [^maxSize := 4]. [MulRdRd] -> [^maxSize := 4]. [DivRdRd] -> [^maxSize := 4]. [SqrtRd] -> [^maxSize := 4]. "Data Movement" [MoveCqR] -> [^maxSize := (operands at: 0) = 0 ifTrue: [2] ifFalse: [5]]. [MoveCwR] -> [^maxSize := 5]. [MoveRR] -> [^maxSize := 2]. [MoveAwR] -> [^maxSize := (self concreteRegister: (operands at: 1)) = EAX ifTrue: [5] ifFalse: [6]]. [MoveRAw] -> [^maxSize := (self concreteRegister: (operands at: 0)) = EAX ifTrue: [5] ifFalse: [6]]. [MoveRMwr] -> [^maxSize := ((self isQuick: (operands at: 1)) ifTrue: [3] ifFalse: [6]) + ((self concreteRegister: (operands at: 2)) = ESP ifTrue: [1] ifFalse: [0])]. [MoveRdM64r] -> [^maxSize := ((self isQuick: (operands at: 1)) ifTrue: [5] ifFalse: [8]) + ((self concreteRegister: (operands at: 2)) = ESP ifTrue: [1] ifFalse: [0])]. [MoveMbrR] -> [^maxSize := ((self isQuick: (operands at: 0)) ifTrue: [3] ifFalse: [6]) + ((self concreteRegister: (operands at: 1)) = ESP ifTrue: [1] ifFalse: [0])]. [MoveRMbr] -> [^maxSize := ((self isQuick: (operands at: 1)) ifTrue: [3] ifFalse: [6]) + ((self concreteRegister: (operands at: 2)) = ESP ifTrue: [1] ifFalse: [0])]. [MoveM16rR] -> [^maxSize := ((self isQuick: (operands at: 0)) ifTrue: [4] ifFalse: [7]) + ((self concreteRegister: (operands at: 1)) = ESP ifTrue: [1] ifFalse: [0])]. [MoveM64rRd] -> [^maxSize := ((self isQuick: (operands at: 0)) ifTrue: [5] ifFalse: [8]) + ((self concreteRegister: (operands at: 1)) = ESP ifTrue: [1] ifFalse: [0])]. [MoveMwrR] -> [^maxSize := ((self isQuick: (operands at: 0)) ifTrue: [3] ifFalse: [6]) + ((self concreteRegister: (operands at: 1)) = ESP ifTrue: [1] ifFalse: [0])]. [MoveXbrRR] -> [self assert: (self concreteRegister: (operands at: 0)) ~= ESP. ^maxSize := (self concreteRegister: (operands at: 1)) = EBP ifTrue: [5] ifFalse: [4]]. [MoveXwrRR] -> [self assert: (self concreteRegister: (operands at: 0)) ~= ESP. ^maxSize := (self concreteRegister: (operands at: 1)) = EBP ifTrue: [4] ifFalse: [3]]. [MoveRXwrR] -> [self assert: (self concreteRegister: (operands at: 1)) ~= ESP. ^maxSize := (self concreteRegister: (operands at: 2)) = EBP ifTrue: [4] ifFalse: [3]]. [PopR] -> [^maxSize := 1]. [PushR] -> [^maxSize := 1]. [PushCw] -> [^maxSize := 5]. [PrefetchAw] -> [^maxSize := self hasSSEInstructions ifTrue: [7] ifFalse: [0]]. "Conversion" [ConvertRRd] -> [^maxSize := 4] }. ^0 "to keep C compiler quiet"
Stef
On Mon, Feb 14, 2011 at 11:00 PM, Stéphane Ducasse <stephane.ducasse@inria.fr> wrote:
Eliot
you use caseOf: for the generation of C in Slang and VM maker. Now this means that - it does not need to be inlined
No. If it is not inlined the simulator will go *much* slower. e.g. CogVMSimulatorLSB>>byteAt: byteAddress | lowBits long | lowBits := byteAddress bitAnd: 3. long := self longAt: byteAddress - lowBits. ^(lowBits caseOf: { [0] -> [ long ]. [1] -> [ long bitShift: -8 ]. [2] -> [ long bitShift: -16 ]. [3] -> [ long bitShift: -24 ] }) bitAnd: 16rFF
so why not put it:
^ (long bitShift: (-8*lowBits) ) bitAnd: 16rFF
? Or this will be slower than caseOf: ?
Because that was the way the code was written. I just copied the method. Further, it is only one example. I'm not going to rewrite the VMMaker's uses of caseOf: jyst to suit some whim of purity. It is making unnecessary work. Taking it out is *much* more work (/and/ emotional energy) than just leaving it alone. Can't we try and be constructive?
- it could be packaged with VMMaker
No. It needs to be in the compiler to be inlined. Why have you got on this hobby-horse? It is a non-issue. caseOf: ios not widelty used but extremely useful in certain circumstances. This has the feeling of a religious pogrom, not a rational approach to the system. IIABDFI = If It Ain't Broke, Don't Fix It.
This concept kinda appeal to me. From other side, i am also strongly feel that house should be kept clean :)
Are these two points correct?
No, IMO, definitely not.
Stef
-- Best regards, Igor Stasenko AKA sig.