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

Eliot Miranda eliot.miranda at gmail.com
Tue Feb 15 21:56:11 CET 2011


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.
> >
> >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gforge.inria.fr/pipermail/pharo-project/attachments/20110215/1aabe322/attachment.htm>


More information about the Pharo-project mailing list