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

Eliot Miranda eliot.miranda at gmail.com
Tue Feb 15 22:12:40 CET 2011


On Tue, Feb 15, 2011 at 1:10 PM, Nicolas Cellier <
nicolas.cellier.aka.nice at gmail.com> wrote:

> 2011/2/15 Eliot Miranda <eliot.miranda at gmail.com>:
> >
> >
> > 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"
>
> In which case the alternative could be to reify Instructions and let
> them respondsTo: maxSize, but one would still need a decoder integer
> -> InstructionClass.
>

That doesn't work in this context because the above has to be translated to
c *without* classes.


> But if it is for the benefit of a single method, it's legitimate to
> wonder why bother and create so many classes.
>
> Nicolas
>
> >>
> >> 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/f1f80935/attachment.htm>


More information about the Pharo-project mailing list