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" <[EMAIL PROTECTED]> wrote in message news:[EMAIL PROTECTED] > 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" <[EMAIL PROTECTED]> wrote in message >> news:[EMAIL PROTECTED] >> 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: [EMAIL PROTECTED] >> For additional commands, e-mail: [EMAIL PROTECTED] >> >> >> >> >> > > -- > Egor Pasko > >
