[LLVMdev] A limitation of LLVM with regard to marking sret functions as readonly.

[Daniel Berlin]

On Mon, Mar 9, 2015 at 3:10 AM, Nicholas Chapman <admin at indigorenderer.com>
wrote:

>  On 08/03/2015 18:07, Daniel Berlin wrote:
>
>
>
> On Sun, Mar 8, 2015 at 9:55 AM, Nicholas Chapman <admin at indigorenderer.com
> > wrote:
>
>> Hi all,
>> I have identified what seems to be a limitation of LLVM with regard to
>> marking 'sret functions' as pure/readonly.
>>
>> For some context - I have some JITed code produced by LLVM, and that code
>> calls back into the host application occasionally.
>> 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.
>> if f is a function exported by the host application,
>> f(1) + f(1)
>> should only call f(1) once, then cache and reuse the value instead of
>> making a second call.
>>
>> 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.
>> 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)
>>
>> There seems to be a similar problem with C++ code:
>>
>> ------------------------------------
>> class s
>> {
>>   public:
>>   float x;
>>   float y;
>> };
>>
>> extern s g(void* env)  __attribute__ ((pure));
>>
>>
>> float func(float x, void* env)
>> {
>>   return g(env).x + g(env).x;
>> }
>> ------------------------------------
>>
>> 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 http://goo.gl/ezXxrI
>>
>> If we make the class bigger, so it gets returned by SRET:
>>
>> ----------------------------------
>> class s
>> {
>>   public:
>>   float x;
>>   float y;
>>
>>   int a;
>>   int b;
>>   int c;
>> };
>>
>> extern s g(void* env)  __attribute__ ((pure));
>>
>>
>> float func(float x, void* env)
>> {
>>   return g(env).x + g(env).x;
>> }
>> ---------------------------------
>>
>> Then LLVM compiles 'func' to have 2 calls to g, and g is no longer marked
>> as 'readonly'  See http://goo.gl/YW0n3V
>
> You mean clang does.
> Because it is clang that is turning off the attributes, deliberately
> (CGCall.cpp)
>
>
>>
>>
> So it doesn't seem possible to me to mark an SRET function as readonly.
>> To me this seems like a problem.
>>
>
>
>  I agree with this one that we should be able to CSE sret'd functions.
>
>
>>
>> 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.
>>
>>
>  Depends on what the actual semantics of readonly are supposed to be.
>
> 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.
> 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.
>
>
>  LLVM has a more well specified definition of readonly:
> "
>  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."
>
>  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.
>
>  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)
>
>  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".
>
>  But if you wanted to make that argument, i feel like you'd have to do
> the legwork.
>
>  (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.
> 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)
>
>    Hi Daniel,
> I think modifying the semantics of readonly would make more sense than
> introducing a new attribute called readonly_except_for_sret.
>

I'm suggesting more "only_writes_return_value" or something.


> As it stands, readonly is completely useless for sret functions.
>

Because they are not actually readonly ;)
Given the choice between shoehorning semantics and introducing new
attributes, i'll generally take introducing new attributes every day.


> It can't be applied to them or the function is not compiled correctly.
> (Which is reasonable given the current readonly semantics)
> I think the best approach would be to modify the readonly semantics, then
> update optimisation passes to respect the new semantics.
>

Why, exactly?


>
> 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.
>

Why?
What is optimizing them and why?


>
> 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.
>
>
I'd file a bug report then.


> Thanks,
>     Nick
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150309/64fc3102/attachment.html>