[PATCH] D104641: [LICM] Strip context sensitive attributes after call hoisting

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

In D104641#2858546 <https://reviews.llvm.org/D104641#2858546>, @nikic wrote:

> Do I understand correctly that a function declaration like `declare void @test(i32 noundef) speculatable` is illegal, because the parameter attributes can result in UB?

Yes, that's right.

> So if the `speculatable` attribute is present, we can assume that all attributes present on the function declaration are also safe to speculate, and the only thing we have to worry about are additional non-speculatable attributes on the call-site?

That's one way to reason about it. So, basically we do not need to be as restrictive as the current patch. For example, if we have the `speculatable` attribute on the function declaration and non null only on the parameter at the callsite, we should be able to run this call anywhere even if the parameter maybe null at the location the call is moved to. The only attributes we should strip are those that cause immediate UB. As Artur pointed out offline, it looks like `nonnull` behaviour was also changed recently to return poison instead of causing UB (if the pointer is indeed null). Given the `speculatable` attribute, we should never have UB within that function if we were to pass in a poison argument.



================
Comment at: llvm/lib/IR/Attributes.cpp:279
+  Attribute::AttrKind A = pImpl->getKindAsEnum();
+  return A == Attribute::SExt || A == Attribute::ZExt; 
+}
----------------
nikic wrote:
> nikic wrote:
> > Shouldn't this also include at least inreg, byval, byref, preallocated, inalloca, sret? Possibly also swiftself/swiftasync and alignstack?
> > 
> > Might make sense to update LangRef with a clearer distinction between ABI and optimization attributes for arguments.
> Though generally, I'm not convinced that we should treat this as dropping all non-abi attributes.
> 
> I think we should only be dropping attributes for which passing an invalid value results in immediate undefined behavior. Dropping `nonnull` for example should not be needed, as that just results in a poison argument, and the `speculatable` attribute promises that the function must deal with that just fine.
> 
> The only attributes we need to drop are things like `noundef`, `dereferenceable` and `dereferenceable_or_null`. Which is pretty much what removeParamUndefImplyingAttributes() is doing (modulo implementation bugs).
agreed. Thanks for the clarification. 


================
Comment at: llvm/lib/IR/Instruction.cpp:185
+  for (unsigned ArgNo = 0; ArgNo < CB->getNumArgOperands(); ArgNo++) {
+    auto PAttr = AL.getParamAttributes(ArgNo); 
+    // List of attributes that are present in parameter that we need to remove.
----------------
nikic wrote:
> What about return attributes? Aren't they also affected?
Yes, that's right. We could move the call anywhere (not just hoist it above conditions affecting the parameter attributes). 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104641/new/

https://reviews.llvm.org/D104641