On 15 February 2011 22:28, Benoit St-Jean <bstjean@yahoo.com> wrote:
Eliot,
Now I fully understand the pain you'd have to go through (and others involved in the VM) if we'd remove this method we don't "like"... I guess it's way easier to live with #caseOf: than live without it...
yeah, in smalltalk you can just reify it to a lot of classes (one for each instruction) and implement #computeMaximumSize per each. Oh.. and you even don't have to do that per each.. if you look at code, it has size=6 for jumps. So, you can implement only 1 method in base 'jump' class, while others will simply reuse it.
CogIA32Compiler>>#computeMaximumSize, when one method is worth a thousand words...
----------------- Benoit St-Jean Yahoo! Messenger: bstjean A standpoint is an intellectual horizon of radius zero. (Albert Einstein)
________________________________ From: Eliot Miranda <eliot.miranda@gmail.com> To: Pharo-project@lists.gforge.inria.fr Sent: Tue, February 15, 2011 3:56:11 PM Subject: Re: [Pharo-project] could we agree to remove caseOf: and caseOf:otherwise:
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.
-- Best regards, Igor Stasenko AKA sig.