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

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:

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.
As it stands, readonly is completely useless for sret functions. 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.

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.

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.

Thanks,
Nick

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

Bug reported filed here:

Thanks so much!