On the 0x21A day of Apache Harmony Konstantin Anisimov wrote: > Egor, > > thank you for the suggestions. I have taken them into account prepareing > next patch version. > Please find it in Jira HARMONY-2061 request.
Konstantin, thank you for updating: * map -> StlMap * removed "Revision" substrings But several issues are not resolved as suggested. Let's discuss those and converge fast. Here they are one by one. Please, comment: * RegOpnd::RegOpnd is still not updated according to the changes in the .h file. Does it compile on IPF? * IpfCodeLayouter.cpp:436: commented code is still there. Is there any reason for that? - 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)); * the relative path is still 'working_vm/vm/jitrino/src/codegenerator/ipf', but 'working_vm' would be better. (this is a minor issue, just for future) > "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 > > > > > > > > -- Egor Pasko
