[PATCH] D20116: Add speculatable function attribute

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

In https://reviews.llvm.org/D20116#713430, @dberlin wrote:

> So from my perspective, the concrete problems you've identified are going to get solved in exactly the same way whether we add them for callsites today, tomorrow, or five years from now.


I agree.

> Hence i see the limitation as fairly artificial. We aren't going solve these problems other than by auditing all the callsite optimizations and either making them drop this attribute, or deeply understand control dependence enough to do a correct thing.

I guess my English isn't strong enough to quite grok that use of "artificial". :)  I partially agree with "We aren't going solve these problems other ... enough to do a correct thing.", but please see below.

>> As I said, my objection was based on my opinion that they're already discovered to be wrong.
> 
> I guess this is where we disagree.
>  I do not believe you have pointed out that there is something wrong with the semantic, at least in a way that you could solve.
>  I believe you have pointed out it will interact badly if we don't audit our code.

What I was *trying* to show was that the context-sensitive-speculatable semantic introduces a fundamentally new behavior -- that dead code can (now) affect the semantics of a program.  I don't think we have this in LLVM today (today the behavior of an LLVM program can be deduced solely from the *trace* of the instructions that were actually executed), and the impact of changing LLVM to allow dead code to have this "action at a distance" is unknown to me.

That ^ is really the core of my objection.  Almost everything else I said can be traced back to the above.

And making dead code affect behavior sets of all sorts of alarms in my head.  I've mentioned some of the more concrete problems earlier, but the fundamental thing that is bugging me is that `if (false) { X }` will not be a NOP for some values of `X`.

I guess what you're saying is that there is nothing fundamental about the dead-code-influencing-behavior problem, and that it is just a matter of fixing passes to do the right thing?

If so, I do not have a better answer to that than a vague sense of unease. :)

> I'd be fine if our answer was "hey, start a patch with all the fixes necessary to make this work properly on callsites".
>  But otherwise, what's the path forward?
>  IE what do you see as a definition of these attributes on callsites that is sane but doesn't have the issues you foresee?

If I was able to sell you on semantically-relevant-dead-code is a bad thing, then there is no path forward with a call specific `speculative` that is as strong as is implied in this patch.  We can probably do something weaker though (say allow it only on calls with all constant arguments).

If you're okay with semantically-relevant-dead-code and its consequences, then it is a matter of fixing all of the passes that assume arbitrary dead code is "okay".

I'm leaning towards the former, but I'd understand if people wanted to do the latter.


https://reviews.llvm.org/D20116