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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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? 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?



================
Comment at: llvm/include/llvm/IR/Instruction.h:397
+  /// attributes that are legal to drop. Both of these should be done by
+  /// passes which moves instructions in IR. 
+  void dropContextSensitiveAttributesAndUnknownMetadata();
----------------
nit: moves -> move


================
Comment at: llvm/lib/IR/Attributes.cpp:279
+  Attribute::AttrKind A = pImpl->getKindAsEnum();
+  return A == Attribute::SExt || A == Attribute::ZExt; 
+}
----------------
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.


================
Comment at: llvm/lib/IR/Attributes.cpp:279
+  Attribute::AttrKind A = pImpl->getKindAsEnum();
+  return A == Attribute::SExt || A == Attribute::ZExt; 
+}
----------------
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).


================
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.
----------------
What about return attributes? Aren't they also affected?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104641