That's fine with me. I've fixed the macro issue and added the inline
keyword, mainly to help document the intent of the code. I'll add the
documentation when a format is decided on.
- Chris
On Wed, Nov 20, 2013 at 3:50 PM, Tom Hildebrandt <[email protected]> wrote:
> Hi Chris:
>
> Thanks for your response.
>
> 1. You are right about the C++ compiler not affecting the C output. I
> was confusing the compiler source with generated code.
>
> Apparently, C++ support for variadic macros came after the C++99 update.
> For the C++ compilers, I think we rely more on the behavior of the set of
> platforms we support than a specific C++ version. Our standard
> configuration (64-bit SUSE Enterprise) uses g++ 4.3.4, which predates the
> C++0x11 standard, and other supported platforms are of an older lineage.
> So I don't think you can expect C++0x11 features to be supported
> uniformly. It's probably best to stay away from them for the time being.
>
> 2. I was just noting a possible difference between a macro vs. static
> implementations of the code. Since my expectation is that g++ will inline
> appropriately even if "inline" is not explicitly specified, there is no
> harm in leaving it off.
>
> 3. I think that that area of the specification (regarding function
> disambiguation) deserves some review. So I will offer to update the
> specification (in collaboration with Jeremy).
>
> 4. Before you spend much time updating the patch, let me get feedback on
> these two open issues (3 and 5).
>
> 5. I will raise the issue of whether we want to use (or at least accept)
> Doxygen for internal documentation of the compiler code.
>
> Many of the team members are attending the SuperComputing conference
> this week, so it may be another week or so before these issues are
> resolved. I hope that is acceptable to you.
>
> Tom H.
>
> ------------------------------
> *From:* Chris Wailes [[email protected]]
> *Sent:* Wednesday, November 20, 2013 12:19 PM
> *To:* Greg Titus
> *Cc:* Tom Hildebrandt; Brad Chamberlain;
> [email protected]
> *Subject:* Re: [Chapel-developers] Request for Review: Refactoring
> Disambiguation By Match
>
>
> 1. The way I've written the macro does appear to use a GNU extension
> (namely, the ## token paste operator). This is necessary because of the
> dangling comma in the call to printf when no arguments are given to the
> variadic macro. While this is cleaner, we can get rid of the GNU extension
> by creating two versions of the tracing macro: one that takes no arguments
> and one that takes a variadic argument.
>
> This does bring up another question I had: what standard are we coding
> to for the C++ portion of the compiler? I would like to use some C++11
> features in functionResolution.cpp, and this shouldn't impact its ability
> to generate C99 code or for the generated code to link against a runtime
> coded in C99.
>
> 2. From my understanding the 'inline' keyword does not ensure that the
> compiler will inline a function. The only thing it requires the compiler
> to do is change the way the function is linked inside the object file.
> While GCC/Clang usually inline functions appropriately, if you REALLY want
> to ensure it is inlined you can use the always_inline function attribute.
> That being said, I don't see any harm in marking this as an inline
> function.
>
> 3. The way the language specification is written the only judgment
> about a function that can be made during disambiguation is whether it is
> "more specific" than another function or not. Therefore, the best you
> could hope for (in the sense of reducing repeated code and better
> performance) is something along the lines of:
>
> if (isMoreSpecificMatch(fn1, fn2, ...) && !isMoreSpecificMatch(fn2,
> fn1, ...)) ...
>
> Unfortunately, even this is impossible with the current
> specification. Because the clauses for determining which function are more
> specific are interleaved (see section 13.14.3 of the specification on page
> 106) you won't get the same result if you do something like above. The
> reason for this is that in the interleaved code the second clause may fire,
> indicating that the second function is a better match, while in the first
> call to isMoreSpecific in the non-interleaved code a later clause fires,
> indicating that the first function is a better match. This later clause
> would never have been reached in the interleaved code, and therefore we
> change the fundamental behavior of the comparison.
>
> Even more restrictive than that, changing the ordering of some of the
> clauses can change the behavior of the comparison. The most extreme
> example of this are clauses 8 and 9 in the argument mapping comparison
> section (page 107). If the ordering of these clauses is changed in any way
> the comparisons will yield very different results.
>
> Something similar to the above example (and what you described) was
> what we were originally going for. Unfortunately it is not possible
> without rewriting the specification, which is something that Jeremy is
> looking into.
>
> 4. I'm certainly willing to refine the patch. I think adding
> documentation is a great idea and we certainly need to figure out what to
> do with the macros. Looking at the old and new versions side by side does
> help though. I would also recommend looking at the new explain-call info
> to see if you think it is clearer.
>
> 5. I would absolutely love to add documentation. Is there any
> objection to me using Doxygen style comments so we could unleash Doxygen on
> the code base at a later date?
>
> Thanks for the feedback.
>
> - Chris
>
>
> On Wed, Nov 20, 2013 at 2:09 PM, Greg Titus <[email protected]> wrote:
>
>> Variadic macros are standard C99, but gcc supports them with slightly
>> different syntax in order to give some additional capabilities, so to
>> maintain portability one has to be careful about how they're written.
>> I haven't looked in detail at the proposed code, so I can't comment as
>> to whether this would pose an issue for the usage there.
>>
>> greg
>>
>>
>>
>> On 11/20/2013 12:46 PM, Tom Hildebrandt wrote:
>>
>>> Hi Chris:
>>>
>>> 1. Variadic macros appear to be a GCC extension. Our base language for
>>> backend
>>> support is C99, so more recent additions to the language cannot be used.
>>> This
>>> is to maintain backend portability.
>>>
>>> 2. I like the conversion of registerParamPreference into a static
>>> function. It
>>> could be marked "inline" to more closely mimic its definition as a macro.
>>> However, GCC is smart enough to figure out whether it can be benefically
>>> inlined, even without the hint.
>>>
>>> 3. I was hoping to see a solution that roughly boiled down to:
>>> score1 = scoreMatch(candidateFn1, info.actuals, actualFormals, scope,
>>> ...);
>>> score2 = scoreMatch(candidateFn2, info.actuals, actualFormals, scope,
>>> ...);
>>> if (score1 < score2) worse = true;
>>> if (score1 == score2) equal = true;
>>>
>>> // Same loop logic
>>>
>>> One could argue that there is some performance gain due to early exit
>>> when the
>>> first candidate is found to be worse. But I doubt it. Integer or boolean
>>> arithmetic is at least 10 times faster than chained decisions. So
>>> replacing
>>> all those "if"s with a simple algebraic expression to compute a score is
>>> likely to be a net win for performance.
>>>
>>> In any case, it is axiomatic that "repeated code" and "early
>>> optimization" are
>>> bug farms, so finding a solution that avoids both will be a boon.
>>>
>>> Another possible implementation is to carry along the set of candidates,
>>> and
>>> apply successively more restrictive filters. If at any point the set of
>>> candidates contains exactly one member, a match has been found. If it
>>> contains
>>> zero, then the result is ambiguity up to the application of the current
>>> filter. (For error reporting, one would maintain a copy of the previous
>>> candidate set, so the conflicting candidates could be listed.)
>>>
>>> 4. That said, I think your factoring still represents an improvement. So
>>> if
>>> you don't feel inclined to rewrite it, we can proceed. Let me know if you
>>> stand behind this patch, so I can apply it and give it a more detailed
>>> review
>>> it "in situ". I have been looking only at the patch file, and might
>>> appreciate
>>> it more by viewing the old and new side by side.
>>>
>>> 5. In any case, it would be helpful if you added a brief header comment
>>> for
>>> each new class you have introduced, describing what it represents and
>>> how it
>>> is used.
>>>
>>> Please respond,
>>> Tom H.
>>>
>>>
>>> ------------------------------------------------------------
>>> ------------------
>>> *From:* Tom Hildebrandt [[email protected]]
>>> *Sent:* Tuesday, November 19, 2013 10:20 PM
>>> *To:* Brad Chamberlain; Chris Wailes; chapel-developers@lists.
>>> sourceforge.net
>>> *Subject:* Re: [Chapel-developers] Request for Review: Refactoring
>>>
>>> Disambiguation By Match
>>>
>>> All:
>>> I have considered rewriting this function, so I may have some useful
>>> insights.
>>> I will review it and post my comments tomorrow.
>>> Tom H.
>>> ------------------------------------------------------------
>>> ------------------
>>> *From:* Brad Chamberlain [[email protected]]
>>> *Sent:* Tuesday, November 19, 2013 9:12 PM
>>> *To:* Chris Wailes; [email protected]
>>> *Subject:* Re: [Chapel-developers] Request for Review: Refactoring
>>>
>>> Disambiguation By Match
>>>
>>> I've been working in this part of the code a lot lately (relatively to
>>> the
>>> rest of the group), and so would be a logical reviewer, and am definitely
>>> interested in reviewing it, but am not certain that I'll have time to
>>> before
>>> the end of Thanksgiving week (and then will have some digging out from
>>> backlog
>>> to do). If someone else from the core team wants to review it in the
>>> meantime,
>>> I can review it post-commit after I get back; or if Chris can wait, I
>>> can do
>>> it then.
>>>
>>> -Brad
>>>
>>>
>>> ------------------------------------------------------------
>>> ------------------
>>> *From:* Chris Wailes [[email protected]]
>>> *Sent:* Tuesday, November 19, 2013 3:50 PM
>>> *To:* [email protected]
>>> *Subject:* [Chapel-developers] Request for Review: Refactoring
>>> Disambiguation
>>>
>>> By Match
>>>
>>> This patch is a refactoring of the disambiguation_by_match function. It
>>> cleans
>>> up the function interface, splits the functionality into several
>>> easier-to-understand functions, and renames variables (and rearranges
>>> some
>>> Boolean conditionals to have the naming make sense) to make their
>>> function
>>> clearer. In addition, information from comparisons is recorded and
>>> reused to
>>> skip over functions that can't be the best match. Lastly, the tracing
>>> enabled
>>> by --explain-call has been improved.
>>>
>>> - Chris
>>>
>>>
>>> ------------------------------------------------------------
>>> ------------------
>>> Shape the Mobile Experience: Free Subscription
>>> Software experts and developers: Be at the forefront of tech innovation.
>>> Intel(R) Software Adrenaline delivers strategic insight and game-changing
>>> conversations that shape the rapidly evolving mobile landscape. Sign up
>>> now.
>>> http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=
>>> /4140/ostg.clktrk
>>>
>>>
>>>
>>> _______________________________________________
>>> Chapel-developers mailing list
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/chapel-developers
>>>
>>
>
------------------------------------------------------------------------------
Shape the Mobile Experience: Free Subscription
Software experts and developers: Be at the forefront of tech innovation.
Intel(R) Software Adrenaline delivers strategic insight and game-changing
conversations that shape the rapidly evolving mobile landscape. Sign up now.
http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk
_______________________________________________
Chapel-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/chapel-developers
