[PATCH] D104641: Strip undef implying attributes when moving calls

[Anna Thomas via Phabricator via llvm-commits]

anna marked an inline comment as done.
anna added inline comments.


================
Comment at: llvm/lib/IR/Attributes.cpp:279
+  Attribute::AttrKind A = pImpl->getKindAsEnum();
+  return A == Attribute::SExt || A == Attribute::ZExt; 
+}
----------------
anna wrote:
> 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. 
Note  that `removeParamUndefImplyingAttributes` does have `Nonnull` in it currently and the comment states that it removes attributes that imply both UB and poison.  
The API was added here: https://reviews.llvm.org/D98899 and there's a comment stating that removing `nonnull` is strictly not necessary but doesn't hurt either. After all, we are propagating poison in that speculatable call.

My updated tests also state the same - we need not remove `nonnull`. I have also added tests to show where we should strictly remove attributes (such as `dereferenceable`).  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104641