[Pharo-project] could we agree to remove caseOf: and caseOf:otherwise:

Stéphane Ducasse stephane.ducasse at inria.fr
Wed Feb 16 08:17:53 CET 2011


But it looks like a DSL to me.
So caseOf: could be moved to Cog.

Stef

On Feb 15, 2011, at 10:28 PM, Benoit St-Jean 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...
> 
> 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 at gmail.com>
> To: Pharo-project at 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 at 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 at 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.
> >
> >
> 
> 
> 
> 
> 





More information about the Pharo-project mailing list