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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1251
+  // have local metadata that might prove the constraint but can't be
+  // speculated).
+
----------------
efriedma wrote:
> 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.
Good point. I hadn't thought about this previously, but we should do something here. D49041 talks about "writes memory visible to the program", and that might not be the best way to describe this. Constant memory is okay, and we need to differentiate this from inaccessiblememonly, which also can read or write memory not accessible to the current module, and at least to me, visible and accessible seem very similar. Moreover, calling this function might still be okay if nothing every actually changes `g` (even if it is mutable)?

Regardless, I agree. If a function is marked as readnone, and contains loads, we should not speculate it.


https://reviews.llvm.org/D49144