<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 06/12/2014 09:25 AM, Owen Anderson
wrote:<br>
</div>
<blockquote
cite="mid:D2D732BD-DF65-42BA-9D10-99F7BC21BDCD@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<br>
<div>
<div>On Jun 12, 2014, at 9:02 AM, Philip Reames <<a
moz-do-not-send="true"
href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 06/12/2014 12:28 AM, Owen
Anderson wrote:<br>
</div>
<blockquote
cite="mid:8244049A-C7C0-44AF-AABE-14AEC6080623@mac.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<br>
<div>
<div>On Jun 11, 2014, at 11:29 PM, deadal nix <<a
moz-do-not-send="true"
href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite"><span style="font-family:
Helvetica; font-size: 12px; font-style: normal;
font-variant: normal; font-weight: normal;
letter-spacing: normal; line-height: normal;
orphans: auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows:
auto; word-spacing: 0px; -webkit-text-stroke-width:
0px; float: none; display: inline !important;">It is
gonna improve the situation quite a lot for all
frontend that use aggregate loads (arguably, that is
a bad practice, but that no reason to stab people in
the back when they do it anyway).</span><br>
</blockquote>
<br>
</div>
<div>I’m not sure I agree with that statement. If we
don’t think they should be used, not optimizing them is
a good way to discourage that. More generally, I’m
concerned about how we will ever get good test coverage
of this code path, since we don’t have any extant front
ends that hit it.</div>
</blockquote>
I'm joining this discussion late, but a) why are aggregate
loads bad practice? Loading something like a small struct
from memory with a single load seems reasonable.</div>
</blockquote>
<div><br>
</div>
<div>They are not well supported through the code generator, and
introducing them would add a lot of complexity. There are
better solutions already in use. The only encouraged use case
for first class structs is as multiple return values.</div>
</div>
</blockquote>
Can you point me to a previous discussion of best practices here?
I've honestly never seen this recommendation.<br>
<br>
When you say "not well supported", do you mean functionally? Or
performance wise? <br>
<br>
Is there an easy lowering scheme which could convert the discourage
form into a preferred form early in the compilation process?<br>
<blockquote
cite="mid:D2D732BD-DF65-42BA-9D10-99F7BC21BDCD@apple.com"
type="cite">
<div><br>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF">b) There are frontends
that are not in tree. The fact that Deadal submitted the
patch is a good hint that *someone* cares.</div>
</blockquote>
<div><br>
</div>
<div>That’s not the same as saying that they *should* care. The
project regularly makes policy and/or technical decisions that
favor good practices over bad. As an extreme example, we
don’t even try to promote non-entry-block allocas to virtual
registers. Totally possible, but we have collectively decided
that the extra complexity is not worth it when we can just
tell frontend authors to adopt best practice and put their
allocas in the entry block. This example is firmly in line
with that.</div>
</div>
</blockquote>
You're making a quite radical claim here. Your comments could be
read to imply that no effort while ever be put into improving things
which are not already used by clang. I know this is not what you
probably meant, but it is the message I'm getting. <br>
<br>
Do you have concrete objections to the patch in question? Looking
through the changes, I see a couple of things which can be improved,
but it seems like a reasonable starting place. It also seems like
a low maintiance burden to trigger this much discussion. :)<br>
<br>
Just to be clear: technical feedback along the lines of "this
doesn't seem like the right approach because of X, Y, Z; have you
considered doing W?" is more than fine. It's the tone and phrasing
I'm objecting to. (And t.m.k. the lack of documentation.)<br>
<br>
Your comments w.r.t. testing is entirely reasonable technical
feedback. <br>
<blockquote
cite="mid:D2D732BD-DF65-42BA-9D10-99F7BC21BDCD@apple.com"
type="cite">
<div>
<div><br>
</div>
<div>Moreover, we generally prefer not to add functionality that
is not exercised in open code. A single regression test is
not good enough. That verifies that one case works, but we’ll
never find the cases that don’t work, or that interact
improperly with other optimizations, because we have no way of
actually testing it live.</div>
</div>
</blockquote>
I would agree that a single regression test is not enough. Having
multiple such tests should be. We routinely take patches found by
analyzing problems with proprietary applications. Claiming that we
don't accept changes which we can't test in their original
environment is simply untrue. <br>
<br>
Just to be clear, I do understand your concerns about how to ensure
good test coverage on changes which don't cover hot paths from
existing well known frontends. Deadal should address this point.
However, a flat rejection is not the right answer if a way to test
the feature adequately can be found.<br>
<br>
Deedal, have you considered trying to find examples where Clang does
emit aggregate loads and stores? Owen mentioned the tuple return
case. Have you considered phrasing optimization in terms of the
benefit there? <br>
<br>
Philip<br>
</body>
</html>