[PATCH] D49144: [FunctionAttrs] Infer the speculatable attribute

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1251
+  // have local metadata that might prove the constraint but can't be
+  // speculated).
+
----------------
hfinkel wrote:
> efriedma wrote:
> > hfinkel wrote:
> > > efriedma wrote:
> > > > Do we also need to look for other attributes on the function?  For example, a function which reads memory but is marked readnone, or a function which calls itself but is marked norecurse.
> > > I looked at them, but I don't think there are any issues in this regard. Because we don't allow branching, there can't be any control dependencies on whether or not memory is read or whether the function recurses. argmemonly, by the current wording, is not allowed to be valid by implicit contract, so even that one is okay.
> > It's possible to have a function which has unconditional undefined behavior; that's fine as long as it isn't "speculatable" and never actually gets called.
> > 
> > Or you could have a function like `int g; const int c = 10; __attribute((const)) int f(bool b) { return *(b ? &g : &c); }`. `f(true)` is UB, but `f(false)` is fine.
> > It's possible to have a function which has unconditional undefined behavior; that's fine as long as it isn't "speculatable" and never actually gets called.
> 
> I completely agree, but we explicitly check the function bodies. I believe that the current checks rule out any UB in the function, even if all arguments (or loaded data, etc.) are poison. Moreover, I don't think that any current function attributes cause a problem in this regard (not that, in theory, an attribute couldn't be added that might need to be handled, just that none of the current ones need handling here).
> 
> > Or you could have a function like ...
> 
> On the general point, I agree. In this case, we'd need to be able to prove all loads dereferenceable in order to speculate. Are you saying that just loading the uninitialized value is UB? (if that's true, then there's another problem with our use of dereferenceable and C++ references, no?)
> 
> 
Simpler testcase: in `int g; __attribute((const)) int f() { return g; }`, it's UB to call the function f, because it returns the value of a mutable global g.  So we can't mark it speculatable.  At least, that's what D49041 says; we could change LangRef, I guess.


https://reviews.llvm.org/D49144