[PATCH] D158081: [IR] Add writable attribute

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1536
+``writable``
+    This attribute is only meaningful in conjunction with ``dereferenceable(N)``
+    or another attribute that implies the first ``N`` bytes of the pointer
----------------
fhahn wrote:
> Can/should this be enforced by the verifier? If yes, might be good to strengthen the wording from `meaningful` to something like ` must be used in conjunction`
I don't think we should verify this bit. Based on past experience, attribute dependencies enforced by the verifier tend to be fairly annoying, so I think we should avoid them when possible. E.g. if we verify this, then ArgPromotion would have to drop the writable attribute when it drops sret -- which seems like an unnecessary code change.

The only reason I added the extra verification for writable + readonly etc is that in that case it's very easy to cause a miscompile if you aren't careful, so the verification seems better than silent UB. Here on the other hand the failure mode is an ignored attribute.


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

https://reviews.llvm.org/D158081