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]<mailto:[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]<mailto:[email protected]>]
*Sent:* Tuesday, November 19, 2013 10:20 PM
*To:* Brad Chamberlain; Chris Wailes;
[email protected]<mailto:[email protected]>
*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]<mailto:[email protected]>]
*Sent:* Tuesday, November 19, 2013 9:12 PM
*To:* Chris Wailes;
[email protected]<mailto:[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]<mailto:[email protected]>]
*Sent:* Tuesday, November 19, 2013 3:50 PM
*To:*
[email protected]<mailto:[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]<mailto:[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
