<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 9, 2015 at 3:10 AM, Nicholas Chapman <span dir="ltr"><<a href="mailto:admin@indigorenderer.com" target="_blank">admin@indigorenderer.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span class="">
On 08/03/2015 18:07, Daniel Berlin wrote:<br>
</span><div><div class="h5"><blockquote type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Sun, Mar 8, 2015 at 9:55 AM,
Nicholas Chapman <span dir="ltr"><<a href="mailto:admin@indigorenderer.com" target="_blank">admin@indigorenderer.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi
all,<br>
I have identified what seems to be a limitation of LLVM
with regard to marking 'sret functions' as pure/readonly.<br>
<br>
For some context - I have some JITed code produced by
LLVM, and that code calls back into the host application
occasionally.<br>
Since my language is purely functional, no functions have
side-effects. Therefore I would like to be able to cache
the value of identical calls back into the host
application, e.g.<br>
if f is a function exported by the host application,<br>
f(1) + f(1)<br>
should only call f(1) once, then cache and reuse the value
instead of making a second call.<br>
<br>
The problem is is that some of the exported functions need
to return structures. As such a pointer to the returned
structure is passed as the first argument and marked with
the sret attribute.<br>
However due to this, that function can no longer be marked
with the 'readonly' attribute (as it needs to write to the
returned structure). (I tried marking the function as
readonly anyway and the function is incorrectly compiled
to 'ret void' or similar)<br>
<br>
There seems to be a similar problem with C++ code:<br>
<br>
------------------------------------<br>
class s<br>
{<br>
public:<br>
float x;<br>
float y;<br>
};<br>
<br>
extern s g(void* env) __attribute__ ((pure));<br>
<br>
<br>
float func(float x, void* env)<br>
{<br>
return g(env).x + g(env).x;<br>
}<br>
------------------------------------<br>
<br>
Compiles the function 'func' to just make a single call to
g. Note that 's' is returned directly instead of being
converted into an SRET arg, due to the small size of the
class 's'. See <a href="http://goo.gl/ezXxrI" target="_blank">http://goo.gl/ezXxrI</a><br>
<br>
If we make the class bigger, so it gets returned by SRET:<br>
<br>
----------------------------------<br>
class s<br>
{<br>
public:<br>
float x;<br>
float y;<br>
<br>
int a;<br>
int b;<br>
int c;<br>
};<br>
<br>
extern s g(void* env) __attribute__ ((pure));<br>
<br>
<br>
float func(float x, void* env)<br>
{<br>
return g(env).x + g(env).x;<br>
}<br>
---------------------------------<br>
<br>
Then LLVM compiles 'func' to have 2 calls to g, and g is
no longer marked as 'readonly' See <a href="http://goo.gl/YW0n3V" target="_blank">http://goo.gl/YW0n3V</a></blockquote>
<div>You mean clang does.</div>
<div>Because it is clang that is turning off the attributes,
deliberately (CGCall.cpp)</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br>
</blockquote>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">So
it doesn't seem possible to me to mark an SRET function as
readonly.<br>
To me this seems like a problem.<br>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<div>I agree with this one that we should be able to CSE
sret'd functions.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
One way to fix this could be to change the semantics of
the readonly attribute - it could be changed to allow
writing through the SRET pointer argument only.<br>
<br>
</blockquote>
<div><br>
</div>
<div>Depends on what the actual semantics of readonly are
supposed to be.</div>
<div><br>
Pure functions in GCC are allowed to write to local,
not-visible-to-caller memory (IIRC, it has been a while),
but not global memory.</div>
<div>So your function is "pure" as defined by GCC (because
it does not access *global* memory, and has no
side-effects). GCC, at least at the level it does most
pure optimization, would see it as byval and never see it
as memory writing.</div>
<div><br>
</div>
<div><br>
</div>
<div>LLVM has a more well specified definition of readonly:<br>
"</div>
<div>
<div>On a function, this attribute indicates that the
function does not write through an pointer arguments
(including byval arguments) or otherwise modify any
state (e.g. memory, control registers, etc) visible to
caller functions. It may dereference pointer arguments
and read state that may be set in the caller. A readonly
function always returns the same value (or unwinds an
exception identically) when called with the same set of
arguments and global state. It cannot unwind an
exception by calling the C++ exception throwing
methods."</div>
<div><br>
</div>
<div>I don't think adding "ps it can write to sret
arguments" would make this definition particularly
sound, but it's certainly arguable you could add "it may
write local stack memory in order to return structure
values only, but may not read that same local memory" or
something.</div>
<div><br>
</div>
<div>But in situations like this, unless you can
essentially prove that no optimizer would ever change as
a result of your definition change (besides it making
theoretical sense), it often makes more sense to add a
new attribute, and add it to the optimizers that can use
it (or abstract it where it makes sense so a new
something like mayBeCSE'd returns true for both readonly
and readonly_except_for_sret, and have the optimizers
use this instead, but the optimizers that care about the
difference can still test the individual flags)</div>
<div><br>
</div>
<div>Now, you are of course, welcome to make such an
argument - go through the optimizers, see what/where
they use readonly, and whether it'd be affected, and say
"actually, they'd all still do the right thing, we
should change readonly".</div>
<div><br>
</div>
<div>But if you wanted to make that argument, i feel like
you'd have to do the legwork.</div>
<div><br>
</div>
<div>(It's probably worth starting by seeing why CGCall
turns off the attributes for sret. i'd find the commit
that did this, and see if there is a bugreport of some
bug this caused, or if it was just done because someone
thought it was wrong without any actual effect.</div>
<div>Because if you get to a bugreport, this will be a
great way to figure out what optimizer/etc would be
affected by a change here)</div>
<div><br>
</div>
</div>
</div>
</div>
</div>
</blockquote></div></div><span class="">
Hi Daniel,<br>
I think modifying the semantics of readonly would make more sense
than introducing a new attribute called readonly_except_for_sret.<br></span></div></blockquote><div><br></div><div>I'm suggesting more "only_writes_return_value" or something.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><span class="">
As it stands, readonly is completely useless for sret functions. </span></div></blockquote><div><br></div><div>Because they are not actually readonly ;)</div><div>Given the choice between shoehorning semantics and introducing new attributes, i'll generally take introducing new attributes every day.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><span class=""> It
can't be applied to them or the function is not compiled correctly.
(Which is reasonable given the current readonly semantics)<br>
I think the best approach would be to modify the readonly semantics,
then update optimisation passes to respect the new semantics.<br></span></div></blockquote><div><br></div><div>Why, exactly?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><span class="">
<br>
I had a look at SVN blame for the CGCall.cpp code, it wasn't very
enlightening however as the last changes were just 'no functional
change' commits. I'm not super keen to dig back further, when the
purpose of the code seems fairly obvious - without stripping the
readonly attribute, sret functions will miscompile.<br></span></div></blockquote><div><br></div><div>Why?<br>What is optimizing them and why?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><span class="">
<br>
I'm not (currently) volunteering to do the work for this in LLVM -
I'm just bringing the issue to the attention of the community.<br>
<br></span></div></blockquote><div><br></div><div>I'd file a bug report then. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><span class="">
Thanks,<br>
Nick<br>
</span></div>
<br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></blockquote></div><br></div></div>