I don't like the ZDL_TARGET ifdef. As is, we have too many #ifdefs. Can you use a different scheme? Sun
On Tue, Sep 6, 2011 at 4:06 PM, Gang <yugang...@gmail.com> wrote: > Hi, JianXin: > > Thanks very much for the review. A revised patch please find in the > attach. > > On 09/06/2011 05:52 PM, Jian-Xin Lai wrote: >> >> Hi Gang, >> >> Sorry for the delay. I have the following comments on this patch: >> >> 1. osprey/be/cg/cgemit.cxx, changes about "Emit_Phase_Validity_Check", I >> prefer to declare this in cgemit.h and provide fake implementation for all >> other platform. FmtAssert FALSE can be used in the fake implementation. The >> same problem about "Target_Specific_ZDLBR_Expansion" in >> osprey/be/cg/whirl2ops.cxx. The major reason is "attribute weak" is not very >> portable. >> > Yes. Both for portability and the intension of keeping unrelated targets > clean, I have changes like the below: > > > +#ifdef ZDL_TARGET > + case OPC_ZDLBR: > + target_tn = Gen_Label_TN (Get_WN_Label (branch), 0); > + /* Target specific Expansion of OPR_ZDLBR */ > + extern void Target_Specific_ZDLBR_Expansion(TN* target_tn); > + Target_Specific_ZDLBR_Expansion(target_tn); > + break; > +#endif > > Thus, for ZDL targets, people have to define the > "Target_Specific_ZDL_Expansion" in the target specific files. For non-ZDL > targets, the code remains untouched. >> >> 2. About TOP_auxbr, is it possible to remove it from the OP_branch or >> OP_cond group in SL's targinfo? So that you don't need to add special code >> to check that TOP. >> > No. Since auxbr is expanded for OPR_ZDLBR, to simulate the *loop* > operator, so we have to setup its branch and cond properties. And it is > because of this, I add some target-specific teaching code in cflow.cxx and > target-specific cgtarget.cxx. > >> 3. In osprey/common/com/opcode_gen_core.h, since OPR_ZDLBR is added for >> all targets, why not move it out of the #ifdef and change the value for the >> later OPRs? >> > This is mainly for maintainance, we can add it as the number of last > commonly shared operators. But the operator sequence of the target-specific > operators are changed. So, wopt may not dump the right output for pre-built > intermediates. >> >> 4. In osprey/common/com/SL/targ_const.cxx, I think Yong-Chong has fixed >> this assertion, the change should be removed from the patch. >> > Yes, his patch does better job. I have removed my change. >> >> 5. In osprey/common/com/SL/config_targ.h, why not define this macro in the >> makefile for SL? > > Good idea. thanks. It makes the config_targ.h cleaner. >> >> 6. It looks like there is no options to skip some of the ZDL >> transformation? is it only controlled by the pragma? >> > For ZDL transformation , we have "WOPT_Enable_ZDL" option to control the > whole zdl transformation. If this is set to on, we assume all > *trip-countable* loops to be transformed to zdl. People may find some loops > do not need such a transform, they are provided with the zdl off pragma. We > could not do the reverse, i.e, disable all transformation and using pragma > zdl on to enable some, since zdl has to be judged by compiler not by human > being. >> >> 7. There are some format issues, especially about the space, tab stop, >> coding style, etc. >> For example, line 224, 271, 406, 425, 446~448, 1602, 2265, 2514, 2526, >> 2582 etc >> > Thanks, please find the change in the attached new patch. > > Gang > > ------------------------------------------------------------------------------ > Malware Security Report: Protecting Your Business, Customers, and the > Bottom Line. Protect your business and customers by understanding the > threat from malware and how it can impact your online business. > http://www.accelacomm.com/jaw/sfnl/114/51427462/ > _______________________________________________ > Open64-devel mailing list > Open64-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/open64-devel > > ------------------------------------------------------------------------------ Malware Security Report: Protecting Your Business, Customers, and the Bottom Line. Protect your business and customers by understanding the threat from malware and how it can impact your online business. http://www.accelacomm.com/jaw/sfnl/114/51427462/ _______________________________________________ Open64-devel mailing list Open64-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/open64-devel
