The major reason I prefer the first one is:
If we define Is_Target_XXX for all platforms, we'll lose some ability to
detect potential problems caused by target-specific changes. The build-time
problem is converted into compile-time, run-time or run-time performance
problem.
2011/7/21 Sun Chan <sun.c...@gmail.com>
> WHy do you like the first solution? It added yet another #ifdef which is
> 1. going against our wish to do fewer #ifdef's
> 2. the whole point of having things like Is_target_xxx() is to do away
> with #ifdef's
>
> Sun
>
> On Wed, Jul 20, 2011 at 3:47 AM, Jian-Xin Lai <laij...@gmail.com> wrote:
> > Actually, I prefer the first patch. Let's wait for Sun's comments on this
> > issue.
> >
> > 2011/7/20 Gang Yu <yugang...@gmail.com>
> >>
> >> Jianxin, Sun;
> >>
> >> I get another solution for this Is_Target_SSE41() problem, I suggest
> a
> >> patch like this:
> >>
> >> --- osprey/common/com/config_lno.cxx (revision 3697)
> >> +++ osprey/common/com/config_lno.cxx (working copy)
> >> @@ -1232,7 +1232,7 @@
> >> the flag is set based on target. Otherwise use user-specified
> value.
> >> */
> >> if(LNO_Iter_threshold == 1) {
> >> - LNO_Iter_threshold = (Is_Target_SSE41())? 8 : 0;
> >> + LNO_Iter_threshold = (Is_Aggressive_SIMD_Target())? 8 : 0;
> >> }
> >> }
> >>
> >> --- osprey/common/com/SL/config_targ.h (revision 3697)
> >> +++ osprey/common/com/SL/config_targ.h (working copy)
> >> @@ -161,6 +161,7 @@
> >> #define Is_Target_ISA_M2Plus() (Target_ISA >= TARGET_ISA_M2)
> >> #define Is_Target_ISA_M3Plus() (Target_ISA >= TARGET_ISA_M3)
> >> #define Is_Target_ISA_M4Plus() (Target_ISA >= TARGET_ISA_M4)
> >> +#define Is_Aggressive_SIMD_Target() (FALSE)
> >>
> >> --- osprey/common/com/x8664/config_targ.h (revision 3697)
> >> +++ osprey/common/com/x8664/config_targ.h (working copy)
> >> @@ -194,8 +194,8 @@
> >> #define Target_x87_precision() (Target_x87_Precision+0)
> >> #define Is_Target_Barcelona() (Target == TARGET_barcelona)
> >> #define Is_Target_Orochi() (Target == TARGET_orochi)
> >> +#define Is_Aggressive_SIMD_Target() (Target_SSE41 == TRUE)
> >>
> >> with this patch, we do not introduce the #ifdef in the target
> independent
> >> sections and also keep the capabilitity to share the code. The patch has
> >> been tested on x86 and SL targets.
> >>
> >> The attached patch also solves other build broken problems for non-x86
> >> targets.
> >>
> >> Please help a review.
> >>
> >> Gang
> >>
> >>
> >> On Mon, Jul 18, 2011 at 4:59 PM, Jian-Xin Lai <laij...@gmail.com>
> wrote:
> >>>
> >>> Hi Sun,
> >>>
> >>> I think Gang's first patch is good enough to fix the compilation error
> on
> >>> both IA-64 and SL. We can let him check in the patch at first and
> continue
> >>> discussing about how to handle the Is_Target_SSE41 (and similar
> macros).
> >>> What's your opinion?
> >>>
> >>> 2011/7/15 Jian-Xin Lai <laij...@gmail.com>
> >>>>
> >>>> I think keeping Is_Target_SSE41 undefined on other platforms will help
> >>>> find any potential problems on other platforms caused by x86 specific
> >>>> changes.
> >>>>
> >>>> For example, in this case, changing Is_Target_SSE41 to return false on
> >>>> other platforms will change the original program's semantic. If we
> >>>> keep Is_Target_SSE41 undefined, we are able to find the problem from
> >>>> the compilation error and check the initial and expected value of
> >>>> LNO_Iter_threshold to make sure if this change will impact the target.
> >>>>
> >>>>
> >>>> 2011/7/12 Sun Chan <sun.c...@gmail.com>:
> >>>> > just return false if it does not apply to you
> >>>> > Sun
> >>>> >
> >>>> > On Tue, Jul 12, 2011 at 9:25 AM, Gang Yu <yugang...@gmail.com>
> wrote:
> >>>> >> Thx, Sun.
> >>>> >>
> >>>> >> Since "Is_Target_SSE41()" is quite a target-specific name, we are
> not
> >>>> >> sure
> >>>> >> of the original author's intension. If this part is sure of a
> >>>> >> target-independent design, we can take this job and we should make
> a
> >>>> >> general
> >>>> >> name for it.
> >>>> >>
> >>>> >> Thanks
> >>>> >>
> >>>> >> Gang
> >>>> >>
> >>>> >>
> >>>> >> On Mon, Jul 11, 2011 at 8:57 PM, Sun Chan <sun.c...@gmail.com>
> wrote:
> >>>> >>>
> >>>> >>> Thx to Gang for pointing out the regression due to v3681 checkin.
> >>>> >>> Here
> >>>> >>> is my suggestion:
> >>>> >>> There are multiple ways to fix the regression that will be seen in
> >>>> >>> other target builds:
> >>>> >>> 1. Fix based on existing infrastructure:
> >>>> >>> Each target must add the corresponding function
> Is_Target_SSE41()
> >>>> >>> in the corresponding config_targ.h
> >>>> >>> I agree with Gang that this fix is not desirable since it
> continues
> >>>> >>> the practice that will cause target dependent build breaks easily
> >>>> >>> and
> >>>> >>> the burden lies with the receiving party, not whoever making the
> >>>> >>> change.
> >>>> >>> 2. Fix that could start a better change to open64 in the longer
> run:
> >>>> >>> Define a target independent Is_Target_SSE41() that will return
> >>>> >>> "false" for all targets, and in the one that matters, return
> result
> >>>> >>> appropriately.
> >>>> >>> One can do this in one of the following two ways I can think
> of:
> >>>> >>> a. define the function with template, and let the "target"
> >>>> >>> specialize that accordingly. This is standard template
> >>>> >>> implementation
> >>>> >>> b. define the generic version as weak, and let the "target"
> >>>> >>> version that the implementor cares about to return the desired
> >>>> >>> result
> >>>> >>> as "strong". This is the implementation most libraries (like libc)
> >>>> >>> use.
> >>>> >>>
> >>>> >>> Now, can I get some volunteer to go about implementing this?
> >>>> >>>
> >>>> >>> Sun
> >>>> >>>
> >>>> >>> On Fri, Jul 8, 2011 at 4:24 PM, Gang Yu <yugang...@gmail.com>
> wrote:
> >>>> >>> > Hi,
> >>>> >>> >
> >>>> >>> > SL target builds fail today due to the LNO check-in v3681,
> the
> >>>> >>> > build
> >>>> >>> > fail
> >>>> >>> > comes from the patch:
> >>>> >>> >
> >>>> >>> > Index: config_lno.cxx
> >>>> >>> >
> >>>> >>> >
> ===================================================================
> >>>> >>> > --- config_lno.cxx (revision 3643)
> >>>> >>> > +++ config_lno.cxx (revision 3681)
> >>>> >>> > @@ -1224,5 +1227,12 @@
> >>>> >>> > Mhd_Options.L[i].TLB_Miss_Penalty;
> >>>> >>> > }
> >>>> >>> > }
> >>>> >>> > +
> >>>> >>> > + /* Value of 1 for LNO_Iter_threshold is interpreted as
> default
> >>>> >>> > in
> >>>> >>> > which
> >>>> >>> > case
> >>>> >>> > + the flag is set based on target. Otherwise use
> >>>> >>> > user-specified
> >>>> >>> > value.
> >>>> >>> > + */
> >>>> >>> > + if(LNO_Iter_threshold == 1) {
> >>>> >>> > + LNO_Iter_threshold = (Is_Target_SSE41())? 8 : 0;
> >>>> >>> > + }
> >>>> >>> > }
> >>>> >>> > Index: /home/yugang/trunk/trunk/osprey/be/cg/whirl2ops.cxx
> >>>> >>> >
> >>>> >>> >
> ===================================================================
> >>>> >>> > --- /home/yugang/trunk/trunk/osprey/be/cg/whirl2ops.cxx
> (revision
> >>>> >>> > 3666)
> >>>> >>> > +++ /home/yugang/trunk/trunk/osprey/be/cg/whirl2ops.cxx
> (revision
> >>>> >>> > 3681)
> >>>> >>> > @@ -3166,9 +3166,18 @@
> >>>> >>> > WN *compare;
> >>>> >>> > VARIANT variant;
> >>>> >>> >
> >>>> >>> > + if (opcode == OPC_V16I1V16I1SELECT) {
> >>>> >>> > + TN* op1 = Expand_Expr(WN_kid0(select), select, NULL);
> >>>> >>> > + TN* op2 = Expand_Expr(WN_kid1(select), select, NULL);
> >>>> >>> > + TN* op3 = Expand_Expr(WN_kid2(select), select, NULL);
> >>>> >>> >
> >>>> >>> > + if (result == NULL)
> >>>> >>> > + result = Allocate_Result_TN (select, NULL);
> >>>> >>> > +
> >>>> >>> > + Expand_Select(result, op1, op2, op3, MTYPE_V16I1, FALSE,
> >>>> >>> > &New_OPs);
> >>>> >>> > //FALSE passed as dummy arg
> >>>> >>> > + return result;
> >>>> >>> > + }
> >>>> >>> >
> >>>> >>> > the "Is_Target_SSE41()", "OPC_V16I1V16I1SELECT" and
> "MTYPE_V16I1"
> >>>> >>> > are
> >>>> >>> > specific to X86 targets, these patches will cause other targets
> >>>> >>> > fail,
> >>>> >>> >
> >>>> >>> > The suggest patche is:
> >>>> >>> > Index: config_lno.cxx
> >>>> >>> >
> >>>> >>> >
> ===================================================================
> >>>> >>> > --- config_lno.cxx (revision 3681)
> >>>> >>> > +++ config_lno.cxx (working copy)
> >>>> >>> > @@ -1228,11 +1228,13 @@
> >>>> >>> > }
> >>>> >>> > }
> >>>> >>> >
> >>>> >>> > +#ifdef TARG_X8664
> >>>> >>> > /* Value of 1 for LNO_Iter_threshold is interpreted as
> default
> >>>> >>> > in
> >>>> >>> > which
> >>>> >>> > case
> >>>> >>> > the flag is set based on target. Otherwise use
> >>>> >>> > user-specified
> >>>> >>> > value.
> >>>> >>> > */
> >>>> >>> > if(LNO_Iter_threshold == 1) {
> >>>> >>> > LNO_Iter_threshold = (Is_Target_SSE41())? 8 : 0;
> >>>> >>> > }
> >>>> >>> > +#endif
> >>>> >>> > }
> >>>> >>> >
> >>>> >>> > Index: ../../be/cg/whirl2ops.cxx
> >>>> >>> >
> >>>> >>> >
> ===================================================================
> >>>> >>> > --- ../../be/cg/whirl2ops.cxx (revision 3681)
> >>>> >>> > +++ ../../be/cg/whirl2ops.cxx (working copy)
> >>>> >>> > @@ -3166,6 +3166,7 @@
> >>>> >>> > WN *compare;
> >>>> >>> > VARIANT variant;
> >>>> >>> >
> >>>> >>> > +#ifdef TARG_X8664
> >>>> >>> > if (opcode == OPC_V16I1V16I1SELECT) {
> >>>> >>> > TN* op1 = Expand_Expr(WN_kid0(select), select, NULL);
> >>>> >>> > TN* op2 = Expand_Expr(WN_kid1(select), select, NULL);
> >>>> >>> > @@ -3177,6 +3178,7 @@
> >>>> >>> > Expand_Select(result, op1, op2, op3, MTYPE_V16I1, FALSE,
> >>>> >>> > &New_OPs);
> >>>> >>> > //FALSE passed as dummy arg
> >>>> >>> > return result;
> >>>> >>> > }
> >>>> >>> > +#endif
> >>>> >>> >
> >>>> >>> >
> >>>> >>> >
> >>>> >>> > Would a gatekeeper help a review?
> >>>> >>> >
> >>>> >>> > Thanks
> >>>> >>> >
> >>>> >>> >
> >>>> >>> > Gang
> >>>> >>> >
> >>>> >>> >
> >>>> >>> >
> >>>> >>> >
> ------------------------------------------------------------------------------
> >>>> >>> > All of the data generated in your IT infrastructure is seriously
> >>>> >>> > valuable.
> >>>> >>> > Why? It contains a definitive record of application performance,
> >>>> >>> > security
> >>>> >>> > threats, fraudulent activity, and more. Splunk takes this data
> and
> >>>> >>> > makes
> >>>> >>> > sense of it. IT sense. And common sense.
> >>>> >>> > http://p.sf.net/sfu/splunk-d2d-c2
> >>>> >>> > _______________________________________________
> >>>> >>> > Open64-devel mailing list
> >>>> >>> > Open64-devel@lists.sourceforge.net
> >>>> >>> > https://lists.sourceforge.net/lists/listinfo/open64-devel
> >>>> >>> >
> >>>> >>> >
> >>>> >>
> >>>> >>
> >>>> >
> >>>> >
> >>>> >
> ------------------------------------------------------------------------------
> >>>> > All of the data generated in your IT infrastructure is seriously
> >>>> > valuable.
> >>>> > Why? It contains a definitive record of application performance,
> >>>> > security
> >>>> > threats, fraudulent activity, and more. Splunk takes this data and
> >>>> > makes
> >>>> > sense of it. IT sense. And common sense.
> >>>> > http://p.sf.net/sfu/splunk-d2d-c2
> >>>> > _______________________________________________
> >>>> > Open64-devel mailing list
> >>>> > Open64-devel@lists.sourceforge.net
> >>>> > https://lists.sourceforge.net/lists/listinfo/open64-devel
> >>>> >
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> Regards,
> >>>> Lai Jian-Xin
> >>>
> >>>
> >>>
> >>> --
> >>> Regards,
> >>> Lai Jian-Xin
> >>
> >
> >
> >
> > --
> > Regards,
> > Lai Jian-Xin
> >
>
--
Regards,
Lai Jian-Xin
------------------------------------------------------------------------------
5 Ways to Improve & Secure Unified Communications
Unified Communications promises greater efficiencies for business. UC can
improve internal communications as well as offer faster, more efficient ways
to interact with customers and streamline customer service. Learn more!
http://www.accelacomm.com/jaw/sfnl/114/51426253/
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
