harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Konstantin Anisimov" <konstantin.m.anisi...@gmail.com>
Subject Re: [drlvm] [ipf] I suggest a series of patches for ipf code generator
Date Tue, 07 Nov 2006 10:35:41 GMT
Egor,

thank you for the suggestions. I have taken them into account prepareing 
next patch version.
Please find it in Jira HARMONY-2061 request.

Thank you,
Konstantin

"Egor Pasko" <egor.pasko@gmail.com> wrote in message 
news:u4ptb7obt.fsf@gmail.com...
> On the 0x216 day of Apache Harmony Konstantin Anisimov wrote:
>> Hi all,
>>
>> I suggest new patch from the series Igor introdusced.
>> 1. To move direct predicated calls in separete node. It allows to have 
>> under
>> predicate short branch instruction instead of call and thus
>>    reduce possible misprediction penalty.
>> 2. I have implemented new node merging algorithm. It is more effective 
>> than
>> previouc one and besides purging empty nodes.
>>
>> All changes made in Code layouting and I suggest integrate them in one
>> patch.
>
> Konstantin,
>
> I took a quick look at the HARMONY-2061 you are introducing. Changes
> look generally good to me, but I have some suggestions (some minor and
> some requiring to update the patch).
>
> Here they are for the easier reply. I'll duplicate them in JIRA for
> the easier tracking.
>
> Changes do *not* affect the IA-32 build. Which is great! (I verified
> this)
>
> I am slightly unhappy with changes like:
>
> ------------------------------------------------------------
> +++ include/IpfCodeLayouter.h   (working copy)
> @@ -17,7 +17,7 @@
>
> /**
>  * @author Intel, Konstantin M. Anisimov, Igor V. Chebykin
> - * @version $Revision$
> + * @version $Revision: 1.1.1.6 $
>  *
>  */
> ------------------------------------------------------------
>
> is it easy to overcome them? maybe, remove the $Revision$ keyword at
> all? too CVS-ish, not for today
>
> Why do you use 'map' instead of more commonly used StlMap
> (MemoryManager based)?  For the sake of safety to memory leakage we
> use memory managers (and allocate them on stack).  I understand that
> your map is allocated on stack, and, hence induces no memory leakage,
> but it is not like the common style we use. Someone can allocate the
> map in heap by mistake.  Could you, please, make it an StlMap?
>
> I see some bugfixes. Cool :)
>
> here:
> ------------------------------------------------------------
> @@ -436,7 +538,8 @@
>     // Add branch to through edge target
>     Opnd    *p0         = cfg.getOpndManager()->getP0();
>     NodeRef *targetNode = cfg.getOpndManager()->newNodeRef(target);
> -    node->addInst(new(mm) Inst(INST_BR, CMPLT_BTYPE_COND, p0, 
> targetNode));
> +//    node->addInst(new(mm) Inst(mm, INST_BR, CMPLT_BTYPE_COND, 
> CMPLT_WH_SPTK, p0, targetNode));
> +    node->addInst(new(mm) Inst(mm, INST_BR, CMPLT_WH_SPTK, CMPLT_PH_FEW, 
> p0, targetNode));
> ------------------------------------------------------------
>
> does it make sense to leave this line commented-out? Please, remove it
> completely.
>
> Minor suggestion: please, provide patches for the "working_vm"
> directory, or one level above, otherwise it needs an extra guess where
> to apply it (this time it is vm/jitrino/src/codegenerator/ipf)
>
> RegOpnd constructor signature changed, but no changes followed in the 
> implementation
> (IpfOpnd.cpp). You probably forgot to include the file into the diff. 
> Please,
> update.
>
> Do I get it right that the new CFG verifier is going to be put in the
> IpfCfgVerifier.{cpp|h}? Is it going to be worked out soon? Who is
> working on it?
>
>> "Igor Chebykin" <iche.harmony@googlemail.com> wrote in message
>> news:e1c5d0b40610200524x5d1fa18di3a348721f2dcba63@mail.gmail.com...
>> Hello all,
>>
>> I suggest a short series of patches for drlvm ipf code generator.
>> We have some improvements for jitrino/ipf
>> and would like to commit its to harmony.
>>
>> All patches will change only vm/jitrino/src/codegenerator/ipf/* files,
>> therefore ia32 remains OK.
>>
>> The first patch is about 67k size and contains following files:
>> IpfCfg.h, IpfCfg.cpp
>>    methods added in Edge and Node classes
>> IpfCodeLayouter.h, IpfCodeLayouter.cpp
>>    new BotomUp algorithm implementation
>> IpfEmitter.h, IpfEmitter.cpp
>>    minor changes in logging, Emitter::registerDirectCall() and
>> debugging support
>> IpfIrPrinter.h, IpfIrPrinter.cpp
>>    added method to print Node chains
>> IpfType.h
>>    types to support Node chains added
>> IpfCfgVerifier.cpp
>>    method cfg.getArgs() deprecated
>> IpfInst.cpp
>>    methods to identify inst kind added (isBr, isCall ?)
>> IpfRegisterAllocator.cpp
>>    minor changes in logging
>>
>> Thanks,
>> Igor.
>>
>>
>> -- 
>> Igor Chebykin, Intel Middleware Products Division
>>
>> ---------------------------------------------------------------------
>> Terms of use : http://incubator.apache.org/harmony/mailing.html
>> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
>> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>>
>>
>>
>>
>>
>
> -- 
> Egor Pasko
>
> 




Mime
View raw message