[IR] Add writable attribute

[nikic]

Summary

This adds a writable attribute, which in conjunction with derferenceable(N) states that a spurious store of N bytes is introduced on function entry. This implies that this many bytes are writable without trapping or introducing data races. See https://llvm.org/docs/Atomics.html#optimization-outside-atomic for why the second point is important.

This attribute can be added to sret arguments. I believe Rust will also be able to use it for by-value (moved) arguments. Rust likely won't be able to use it for &mut arguments (tree borrows does not appear to allow spurious stores).

In this patch the new attribute is only used by LICM scalar promotion. However, the actual motivation for this is to fix a correctness issue in call slot optimization, which needs this attribute to avoid optimization regressions.

Followup to the discussion on D157499.

[nikic]

Forgot to git add one file.

[RalfJung]

For that reason the writable attribute is specified to perform a spurious store of dereferenceable bytes on function entry.

That makes a lot of sense; FWIW I also don't know any better way to specify the regular dereferenceable attribute (i.e., as spurious reads). In particular those accesses are relevant for noalias. See here for me exploring this in the Rust context.

For this particular attribute however, just to be sure: it is still the responsibility of the optimization to ensure that the spuriously written value does not cause wrong results elsewhere, right? In other words, an empty function with a pointer argument that is 4-byte writeable can *not* just get a spurious store of 0, since someone might actually look at that memory after the function returns and they would be able to see the difference?

[nikic]

In D158081#4594615, @RalfJung wrote:

For that reason the writable attribute is specified to perform a spurious store of dereferenceable bytes on function entry.

That makes a lot of sense; FWIW I also don't know any better way to specify the regular dereferenceable attribute (i.e., as spurious reads). In particular those accesses are relevant for noalias. See here for me exploring this in the Rust context.

For this particular attribute however, just to be sure: it is still the responsibility of the optimization to ensure that the spuriously written value does not cause wrong results elsewhere, right? In other words, an empty function with a pointer argument that is 4-byte writeable can *not* just get a spurious store of 0, since someone might actually look at that memory after the function returns and they would be able to see the difference?

That's right. The attribute always allows you to introduce *p = *p on function entry (in conjunction with dereferenceable), but introducing *p = 0 would require you to prove that this does not change observable behavior (e.g. because you're moving up a later *p = 0 store and there are no reads/unwinds in between). Another caveat is that once you move away from function entry, you also need to ensure that the pointer does not escape to a different thread.

[RalfJung]

introduce *p = *p on function entry

I'm not sure about that. If some but not all of the bytes at *p were poison, then after this *p = *p they will all be poison (since *p will turn a single-byte poison into a whole-value poison).

If there was a "bytes" type that would preserve per-byte poison then this might be possible, but with the whole-value poison of the integer types I don't think this works.

[nikic]

In D158081#4594727, @RalfJung wrote:

introduce *p = *p on function entry

I'm not sure about that. If some but not all of the bytes at *p were poison, then after this *p = *p they will all be poison (since *p will turn a single-byte poison into a whole-value poison).

The notation here was a bit too abstract. It would be more precise to say that you can llvm.memcpy(p, p, dereferenceable(p)).

The poison propagation issue is a very general one -- and indeed, it does also appear when introducing spurious stores. It's a well known problem that we currently don't have a good answer to. (The byte type will likely be part of that answer.)

[RalfJung]

Makes sense, thanks!

[aeubanks]

can you explain the data race part more in the description?

[nikic]

In D158081#4595846, @aeubanks wrote:

can you explain the data race part more in the description?

This section from the atomic optimization guide describes the data race issue in some more detail: https://llvm.org/docs/Atomics.html#optimization-outside-atomic

[efriedma]

llvm/test/Transforms/LICM/scalar-promote.ll

889

I'm not sure this transform is actually legal without dereferenceable(4)? The way "writable" is specified, no "deferenceable" means no phantom store, so this is introducing a race.

[nikic]

llvm/test/Transforms/LICM/scalar-promote.ll

889

sret(%T) implies dereferenceable(sizeof(%T)). I should probably tweak the wording to not talk about dereferenceable(N) specifically but attributes implying dereferenceability in general.

(And we should really drop the type argument from sret and make people specify align and dereferenceable instead...)

[efriedma]

llvm/test/Transforms/LICM/scalar-promote.ll

889

Is that actually the way the compiler is reasoning about it? Can you add a negative test showing the sret is actually necessary here?

[nikic]

Pivot the definition to only be meaningful in conjunction with dereferenceable (or attributes implying it). Just the writability property without the data-race freedom doesn't seem particularly useful, and we only get the data race freedom with dereferenceable, so it's probably simpler to just couple these directly. (It would be nice to not have the coupling, but I don't really see a way to specify that operationally.)

Adjust the use in LICM to explicitly check for dereferenceability if required.

[nikic]

llvm/test/Transforms/LICM/scalar-promote.ll

889

Yes, you were right and this did not reason based on explicit dereferenceability. I've added a test with an insufficient dereferenceable annotation and adjusted the implementation. I've also adjusted the LangRef wording to tie it to explicit dereferenceability.

[efriedma]

I think there's a natural connection to dereferenceable here: "dereferenceable(N)" loads N bytes on entry, "deferenceable(N) writable" loads and stores N bytes on entry. So gluing them together makes sense to me.

llvm/docs/LangRef.rst

1537

I think I'd suggest dropping the "or another attribute" part; it makes it harder to reason about the semantics on the caller side.

[jdoerfert]

I think this is fine. I still think we might just want to add this as a modifier in dereferenceable (if it is tied anyway): dereferenceable(writeable: N).
Again, the motivation stems also from the fact that we could then make dereferenceable_globally another modifier: dereferenceable(globally[, writeable]: N).

[nikic]

llvm/docs/LangRef.rst

1537

My motivation for this is to allow reusing standard dereferenceability APIs. Otherwise we would have to thread an extra option through APIs like isDereferenceablePointer() that restricts it to only dereferenceable and excludes the other attributes.

[efriedma]

llvm/docs/LangRef.rst

1537

Value::getPointerDereferenceableBytes() isn't that complicated; we can write out the other cases it handles here, I guess. I'm more worried about it being open-ended.

To be more specific about what I'm worried about, the exact number of bytes written is relevant for a couple of reasons. One, alias analysis in the caller: ideally, the caller should be able to understand which bytes are modified (although I guess that might require attributes that don't currently exist). And two, any transform that modifies a relevant attribute needs to worry about whether the transform invalidates the "writable" attribute.

[nikic]

Something I'm concerned about is the interaction between writable and readonly, as well as memory(argmem: read). The way this is specified right now, writable readonly is immediate UB. And I think that's semantically correct.

However, I think it would be a good idea to make things like writable readonly a verifier error instead. This will make sure that passes like FuncAttrs remove the writable attribute when inferring readonly (which this patch currently fails to do). I assume we prefer having readonly over writable, as in the cases where we can infer readonly we wouldn't be interested in inserting spurious stores anyway.

[nikic]

Forbid writable + readonly and similar combinations.

[nikic]

llvm/docs/LangRef.rst

1537

I've added an explicit list of the attributes.

I think in terms of inference the case to be concerned about is when dereferenceable with a larger byte range is inferred (maybe Attributor does that, haven't checked). The rest are ABI attributes and we shouldn't infer those.

How would the caller use writable for alias analysis?

[efriedma]

llvm/docs/LangRef.rst

1537

The presence of a non-atomic write allows other non-atomic writes to be introduced; LICM could use that in theory. Maybe too niche to really be useful.

At some point, we could add an attribute on calls to indicate the maximum number of bytes written through an argument; if we have that, making sure we don't write past the limit would become more important.

1550

Does anything actually use sret to drive analysis like this? I thought we eliminated most of the usage.

[nikic]

llvm/docs/LangRef.rst

1550

It happens here: https://github.com/llvm/llvm-project/blob/f105ed11ae44b382f1c2cf9f83cba313ad0b8d7f/llvm/lib/IR/Value.cpp#L857-L863 sret is one of the types part of getPointeeInMemoryValueType().

Worth noting that clang doesn't currently emit dereferenceable for sret arguments, it relies on the implied dereferenceability.

[efriedma]

I'm not really comfortable reviewing the attributor stuff, but the rest looks fine to me.

[nikic]

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

8038

@jdoerfert What would be the correct way to remove an attribute from all parameters here?

In practice just the handling above seems to be enough, but it would be cleaner to explicitly remove writable if we infer readonly here.

[jdoerfert]

llvm/docs/LangRef.rst

1537

maximum number of bytes

We never landed objectsize, which is similar, and 'maxwrite' would also be good, esp. now that we can shrink allocations and track content of memory.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

7849

one implies the other. Maybe use a precompiled set, like AttrKinds above, the call to remove is not free.

8038

You can just add it to AttrKinds that is passed to removeAttrs, no?

[nikic]

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

8038

The problem is that Memory is a function attribute, while Writable is a parameter attribute, so I think the getIRPosition() would be the wrong one for it?

[jdoerfert]

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

8038

Oh, then u need to iterate over all arguments I guess. Not that we forbid readnone on the function and writeonly on an argument right now.

[nikic]

Drop conflicting attribute in attributor.

[nikic]

ping

[nikic]

ping :)

[nikic]

ping!

[fhahn]

llvm/docs/LangRef.rst

1536

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

[nikic]

llvm/docs/LangRef.rst

1536

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.

[nikic]

ping ;)

[nikic]

ping...

[goldstein.w.n]

llvm/lib/Transforms/Scalar/LICM.cpp

2177

nit: Given that the writeable attribute is only meaningful in conjunction with isDeferenceablePointer, maybe that check should just be part of the isWritableObject function? Then you can drop the ExplicitlyDeferenceableOnly bool. Just in general seems easier on the user to have the function do the checks of whether the writable attribute is meaningful for the object rather than the otherway around.

In general though I don't see any issues with the code, but am in no way qualified to approve this patch.

[nikic]

llvm/lib/Transforms/Scalar/LICM.cpp

2177

My thinking here was that some callers will have already performed the isDereferenceablePointer check anyway, so we can avoid doing it again in isWritableObject. In particular performCallSlotOptzn() already checks isDereferenceableAndAlignedPointer().

[nikic]

Rebase & ping

[aeubanks]

llvm/docs/LangRef.rst

1540–1541

is this part necessary? or can we just defer to the next paragraph for what this attribute allows?

[nikic]

llvm/docs/LangRef.rst

1540–1541

I think so. This is the part specifying the actual operational semantics, the rest is just implications. This gives a general answer to some non-obvious questions, such as "if I have two aliasing noalias arguments that are only read inside the function, but one of them is marked writable, is this UB?" to which the answer would be "yes, because writable implies a write on entry, which makes aliasing noalias UB".

[efriedma]

LGTM

[aeubanks]

llvm/docs/LangRef.rst

1544

perhaps link to https://llvm.org/docs/Atomics.html#optimization-outside-atomic

llvm/docs/LangRef.rst

 ``writeonly``
     This attribute indicates that the function may write to, but does not read
     through this pointer argument (even though it may read from the memory that
     the pointer points to).
 
     If a function reads from a writeonly pointer argument, the behavior is
     undefined.
 
+``writable``
+    This attribute is only meaningful in conjunction with ``dereferenceable(N)``
+    or another attribute that implies the first ``N`` bytes of the pointer
+    argument are dereferenceable.
+
+    In that case, the attribute indicates that the first ``N`` bytes will be
+    (non-atomically) loaded and stored back on entry to the function.
+
+    This implies that it's possible to introduce spurious stores on entry to
+    the function without introducing traps or data races. This does not
+    necessarily hold throughout the whole function, as the pointer may escape
+    to a different thread during the execution of the function. See also the
+    :ref:`atomic optimization guide <Optimization outside atomic>`
+
+    The "other attributes" that imply dereferenceability are
+    ``dereferenceable_or_null`` (if the pointer is non-null) and the
+    ``sret``, ``byval``, ``byref``, ``inalloca``, ``preallocated`` family of
+    attributes. Note that not all of these combinations are useful, e.g.
+    ``byval`` arguments are known to be writable even without this attribute.
+
+    The ``writable`` attribute cannot be combined with ``readnone``,
+    ``readonly`` or a ``memory`` attribute that does not contain
+    ``argmem: write``.
+
 .. _gc:
 
 Garbage Collector Strategy Names
 --------------------------------
 
 Each function may specify a garbage collector strategy name, which is simply a
 string:
 

llvm/include/llvm/Analysis/AliasAnalysis.h

 /// value on unwind. If the RequiresNoCaptureBeforeUnwind out parameter is set
 /// to true, then the memory is only not visible if the object has not been
 /// captured prior to the unwind. Otherwise it is not visible even if captured.
 bool isNotVisibleOnUnwind(const Value *Object,
                           bool &RequiresNoCaptureBeforeUnwind);
 
 /// Return true if the Object is writable, in the sense that any location based
 /// on this pointer that can be loaded can also be stored to without trapping.
-///
-/// By itself, this does not imply that introducing spurious stores is safe,
-/// for example due to thread-safety reasons.
-bool isWritableObject(const Value *Object);
+/// Additionally, at the point Object is declared, stores can be introduced
+/// without data races. At later points, this is only the case if the pointer
+/// can not escape to a different thread.
+///
+/// If ExplicitlyDereferenceableOnly is set to true, this property only holds
+/// for the part of Object that is explicitly marked as dereferenceable, e.g.
+/// using the dereferenceable(N) attribute. It does not necessarily hold for
+/// parts that are only known to be dereferenceable due to the presence of
+/// loads.
+bool isWritableObject(const Value *Object, bool &ExplicitlyDereferenceableOnly);
 
 /// A manager for alias analyses.
 ///
 /// This class can have analyses registered with it and when run, it will run
 /// all of them and aggregate their results into single AA results interface
 /// that dispatches across all of the alias analysis results available.
 ///
 /// Note that the order in which analyses are registered is very significant.

llvm/include/llvm/Bitcode/LLVMBitCodes.h

enum AttributeKindCodes {
   ATTR_KIND_ALLOCATED_POINTER = 81,
   ATTR_KIND_ALLOC_KIND = 82,
   ATTR_KIND_PRESPLIT_COROUTINE = 83,
   ATTR_KIND_FNRETTHUNK_EXTERN = 84,
   ATTR_KIND_SKIP_PROFILE = 85,
   ATTR_KIND_MEMORY = 86,
   ATTR_KIND_NOFPCLASS = 87,
   ATTR_KIND_OPTIMIZE_FOR_DEBUGGING = 88,
+  ATTR_KIND_WRITABLE = 89,
 };
 
 enum ComdatSelectionKindCodes {
   COMDAT_SELECTION_KIND_ANY = 1,
   COMDAT_SELECTION_KIND_EXACT_MATCH = 2,
   COMDAT_SELECTION_KIND_LARGEST = 3,
   COMDAT_SELECTION_KIND_NO_DUPLICATES = 4,
   COMDAT_SELECTION_KIND_SAME_SIZE = 5,

llvm/include/llvm/IR/Attributes.td

 def UWTable : IntAttr<"uwtable", [FnAttr]>;
 
 /// Minimum/Maximum vscale value for function.
 def VScaleRange : IntAttr<"vscale_range", [FnAttr]>;
 
 /// Function always comes back to callsite.
 def WillReturn : EnumAttr<"willreturn", [FnAttr]>;
 
+/// Pointer argument is writable.
+def Writable : EnumAttr<"writable", [ParamAttr]>;
+
 /// Function only writes to memory.
 def WriteOnly : EnumAttr<"writeonly", [ParamAttr]>;
 
 /// Zero extended before/after call.
 def ZExt : EnumAttr<"zeroext", [ParamAttr, RetAttr]>;
 
 /// Function is required to make Forward Progress.
 def MustProgress : EnumAttr<"mustprogress", [FnAttr]>;

llvm/lib/Analysis/AliasAnalysis.cpp

if (isNoAliasCall(Object)) {
     return true;
   }
 
   return false;
 }
 
 // We don't consider globals as writable: While the physical memory is writable,
 // we may not have provenance to perform the write.
-bool llvm::isWritableObject(const Value *Object) {
+bool llvm::isWritableObject(const Value *Object,
+                            bool &ExplicitlyDereferenceableOnly) {
+  ExplicitlyDereferenceableOnly = false;
+
   // TODO: Alloca might not be writable after its lifetime ends.
   // See https://github.com/llvm/llvm-project/issues/51838.
   if (isa<AllocaInst>(Object))
     return true;
 
-  // TODO: Also handle sret.
-  if (auto *A = dyn_cast<Argument>(Object))
+  if (auto *A = dyn_cast<Argument>(Object)) {
+    if (A->hasAttribute(Attribute::Writable)) {
+      ExplicitlyDereferenceableOnly = true;
+      return true;
+    }
+
     return A->hasByValAttr();
+  }
 
   // TODO: Noalias shouldn't imply writability, this should check for an
   // allocator function instead.
   return isNoAliasCall(Object);
 }

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
   case bitc::ATTR_KIND_BYREF:
     return Attribute::ByRef;
   case bitc::ATTR_KIND_MUSTPROGRESS:
     return Attribute::MustProgress;
   case bitc::ATTR_KIND_HOT:
     return Attribute::Hot;
   case bitc::ATTR_KIND_PRESPLIT_COROUTINE:
     return Attribute::PresplitCoroutine;
+  case bitc::ATTR_KIND_WRITABLE:
+    return Attribute::Writable;
   }
 }
 
 Error BitcodeReader::parseAlignmentValue(uint64_t Exponent,
                                          MaybeAlign &Alignment) {
   // Note: Alignment in bitcode files is incremented by 1, so that zero
   // can be used for default alignment.
   if (Exponent > Value::MaxAlignmentExponent + 1)

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
   case Attribute::NoUndef:
     return bitc::ATTR_KIND_NOUNDEF;
   case Attribute::ByRef:
     return bitc::ATTR_KIND_BYREF;
   case Attribute::MustProgress:
     return bitc::ATTR_KIND_MUSTPROGRESS;
   case Attribute::PresplitCoroutine:
     return bitc::ATTR_KIND_PRESPLIT_COROUTINE;
+  case Attribute::Writable:
+    return bitc::ATTR_KIND_WRITABLE;
   case Attribute::EndAttrKinds:
     llvm_unreachable("Can not encode end-attribute kinds marker.");
   case Attribute::None:
     llvm_unreachable("Can not encode none-attribute.");
   case Attribute::EmptyKey:
   case Attribute::TombstoneKey:
     llvm_unreachable("Trying to encode EmptyKey/TombstoneKey");
   }

llvm/lib/IR/Attributes.cpp

if (!Ty->isPointerTy()) {
     // Attributes that only apply to pointers.
     if (ASK & ASK_SAFE_TO_DROP)
       Incompatible.addAttribute(Attribute::NoAlias)
           .addAttribute(Attribute::NoCapture)
           .addAttribute(Attribute::NonNull)
           .addAttribute(Attribute::ReadNone)
           .addAttribute(Attribute::ReadOnly)
           .addAttribute(Attribute::Dereferenceable)
-          .addAttribute(Attribute::DereferenceableOrNull);
+          .addAttribute(Attribute::DereferenceableOrNull)
+          .addAttribute(Attribute::Writable);
     if (ASK & ASK_UNSAFE_TO_DROP)
       Incompatible.addAttribute(Attribute::Nest)
           .addAttribute(Attribute::SwiftError)
           .addAttribute(Attribute::Preallocated)
           .addAttribute(Attribute::InAlloca)
           .addAttribute(Attribute::ByVal)
           .addAttribute(Attribute::StructRet)
           .addAttribute(Attribute::ByRef)

llvm/lib/IR/Verifier.cpp

Check(!(Attrs.hasAttribute(Attribute::ReadOnly) &&
         V);
 
   Check(!(Attrs.hasAttribute(Attribute::NoInline) &&
           Attrs.hasAttribute(Attribute::AlwaysInline)),
         "Attributes "
         "'noinline and alwaysinline' are incompatible!",
         V);
 
+  Check(!(Attrs.hasAttribute(Attribute::Writable) &&
+          Attrs.hasAttribute(Attribute::ReadNone)),
+        "Attributes writable and readnone are incompatible!", V);
+
+  Check(!(Attrs.hasAttribute(Attribute::Writable) &&
+          Attrs.hasAttribute(Attribute::ReadOnly)),
+        "Attributes writable and readonly are incompatible!", V);
+
   AttributeMask IncompatibleAttrs = AttributeFuncs::typeIncompatible(Ty);
   for (Attribute Attr : Attrs) {
     if (!Attr.isStringAttribute() &&
         IncompatibleAttrs.contains(Attr.getKindAsEnum())) {
       CheckFailed("Attribute '" + Attr.getAsString() +
                   "' applied to incompatible type!", V);
       return;
     }
void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
   if (Attrs.hasFnAttr(Attribute::OptimizeForDebugging)) {
     Check(!Attrs.hasFnAttr(Attribute::OptimizeForSize),
           "Attributes 'optsize and optdebug' are incompatible!", V);
 
     Check(!Attrs.hasFnAttr(Attribute::MinSize),
           "Attributes 'minsize and optdebug' are incompatible!", V);
   }
 
+  Check(!Attrs.hasAttrSomewhere(Attribute::Writable) ||
+        isModSet(Attrs.getMemoryEffects().getModRef(IRMemLocation::ArgMem)),
+        "Attribute writable and memory without argmem: write are incompatible!",
+        V);
+
   if (Attrs.hasFnAttr("aarch64_pstate_sm_enabled")) {
     Check(!Attrs.hasFnAttr("aarch64_pstate_sm_compatible"),
            "Attributes 'aarch64_pstate_sm_enabled and "
            "aarch64_pstate_sm_compatible' are incompatible!",
            V);
   }
 
   if (Attrs.hasFnAttr("aarch64_pstate_za_new")) {

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

ChangeStatus manifest(Attributor &A) override {
     if (llvm::all_of(DeducedAttrs, [&](const Attribute &Attr) {
           return A.hasAttr(IRP, Attr.getKindAsEnum(),
                            /* IgnoreSubsumingPositions */ true);
         }))
       return ChangeStatus::UNCHANGED;
 
     // Clear existing attributes.
     A.removeAttrs(IRP, AttrKinds);
+    // Clear conflicting writable attribute.
+    if (isAssumedReadOnly())
+      A.removeAttrs(IRP, Attribute::Writable);
 
     // Use the generic manifest method.
     return IRAttribute::manifest(A);
   }
 
   /// See AbstractState::getAsStr().
   const std::string getAsStr(Attributor *A) const override {
     if (isAssumedReadNone())
ChangeStatus manifest(Attributor &A) override {
     if (isAssumedReadNone())
       ME = MemoryEffects::none();
     else if (isAssumedReadOnly())
       ME = MemoryEffects::readOnly();
     else if (isAssumedWriteOnly())
       ME = MemoryEffects::writeOnly();
 
     A.removeAttrs(getIRPosition(), AttrKinds);
+    // Clear conflicting writable attribute.
+    if (ME.onlyReadsMemory())
+      for (Argument &Arg : F.args())
+        A.removeAttrs(IRPosition::argument(Arg), Attribute::Writable);
     return A.manifestAttrs(getIRPosition(),
                            Attribute::getWithMemoryEffects(F.getContext(), ME));
   }
 
   /// See AbstractAttribute::trackStatistics()
   void trackStatistics() const override {
     if (isAssumedReadNone())
       STATS_DECLTRACK_FN_ATTR(readnone)
ChangeStatus manifest(Attributor &A) override {
     if (isAssumedReadNone())
       ME = MemoryEffects::none();
     else if (isAssumedReadOnly())
       ME = MemoryEffects::readOnly();
     else if (isAssumedWriteOnly())
       ME = MemoryEffects::writeOnly();
 
     A.removeAttrs(getIRPosition(), AttrKinds);
+    // Clear conflicting writable attribute.
+    if (ME.onlyReadsMemory())
+      for (Use &U : CB.args())
+        A.removeAttrs(IRPosition::callsite_argument(CB, U.getOperandNo()),
+                      Attribute::Writable);
     return A.manifestAttrs(
         getIRPosition(), Attribute::getWithMemoryEffects(CB.getContext(), ME));
   }
 
   /// See AbstractAttribute::trackStatistics()
   void trackStatistics() const override {
     if (isAssumedReadNone())
       STATS_DECLTRACK_CS_ATTR(readnone)

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

if (ArgMR != ModRefInfo::NoModRef)
     ME |= RecursiveArgME & MemoryEffects(ArgMR);
 
   for (Function *F : SCCNodes) {
     MemoryEffects OldME = F->getMemoryEffects();
     MemoryEffects NewME = ME & OldME;
     if (NewME != OldME) {
       ++NumMemoryAttr;
       F->setMemoryEffects(NewME);
+      // Remove conflicting writable attributes.
+      if (!isModSet(NewME.getModRef(IRMemLocation::ArgMem)))
+        for (Argument &A : F->args())
+          A.removeAttr(Attribute::Writable);
       Changed.insert(F);
     }
   }
 }
 
 // Compute definitive function attributes for a function taking into account
 // prevailing definitions and linkage types
 static FunctionSummary *calculatePrevailingSummary(
static bool addAccessAttr(Argument *A, Attribute::AttrKind R) {
   if (A->hasAttribute(R))
       return false;
 
   // Otherwise, remove potentially conflicting attribute, add the new one,
   // and update statistics.
   A->removeAttr(Attribute::WriteOnly);
   A->removeAttr(Attribute::ReadOnly);
   A->removeAttr(Attribute::ReadNone);
+  // Remove conflicting writable attribute.
+  if (R == Attribute::ReadNone || R == Attribute::ReadOnly)
+    A->removeAttr(Attribute::Writable);
   A->addAttr(R);
   if (R == Attribute::ReadOnly)
     ++NumReadOnlyArg;
   else if (R == Attribute::WriteOnly)
     ++NumWriteOnlyArg;
   else
     ++NumReadNoneArg;
   return true;

llvm/lib/Transforms/Scalar/LICM.cpp

bool llvm::promoteLoopAccessesToScalars(
   }
 
   // We know we can hoist the load, but don't have a guaranteed store.
   // Check whether the location is writable and thread-local. If it is, then we
   // can insert stores along paths which originally didn't have them without
   // violating the memory model.
   if (StoreSafety == StoreSafetyUnknown) {
     Value *Object = getUnderlyingObject(SomePtr);
-    if (isWritableObject(Object) &&
+    bool ExplicitlyDereferenceableOnly;
+    if (isWritableObject(Object, ExplicitlyDereferenceableOnly) &&
+        (!ExplicitlyDereferenceableOnly ||
+         isDereferenceablePointer(SomePtr, AccessTy, MDL)) &&
         isThreadLocalObject(Object, CurLoop, DT, TTI))
       StoreSafety = StoreSafe;
   }
 
   // If we've still failed to prove we can sink the store, hoist the load
   // only, if possible.
   if (StoreSafety != StoreSafe && !FoundLoadToPromote)
     // If we cannot hoist the load either, give up.

llvm/lib/Transforms/Utils/CodeExtractor.cpp

if (Attr.isStringAttribute()) {
       case Attribute::StructRet:
       case Attribute::SwiftError:
       case Attribute::SwiftSelf:
       case Attribute::SwiftAsync:
       case Attribute::ZExt:
       case Attribute::ImmArg:
       case Attribute::ByRef:
       case Attribute::WriteOnly:
+      case Attribute::Writable:
       //  These are not really attributes.
       case Attribute::None:
       case Attribute::EndAttrKinds:
       case Attribute::EmptyKey:
       case Attribute::TombstoneKey:
         llvm_unreachable("Not a function attribute");
       }
 

llvm/test/Bitcode/attributes.ll

 }
 
 ; CHECK: define void @f87() [[FNRETTHUNKEXTERN:#[0-9]+]]
 define void @f87() fn_ret_thunk_extern { ret void }
 
 ; CHECK: define void @f88() [[SKIPPROFILE:#[0-9]+]]
 define void @f88() skipprofile { ret void }
 
-define void @f89() optdebug
 ; CHECK: define void @f89() [[OPTDEBUG:#[0-9]+]]
-{
+define void @f89() optdebug {
         ret void;
 }
 
+; CHECK: define void @f90(ptr writable %p)
+define void @f90(ptr writable %p) {
+  ret void
+}
+
 ; CHECK: attributes #0 = { noreturn }
 ; CHECK: attributes #1 = { nounwind }
 ; CHECK: attributes #2 = { memory(none) }
 ; CHECK: attributes #3 = { memory(read) }
 ; CHECK: attributes #4 = { noinline }
 ; CHECK: attributes #5 = { alwaysinline }
 ; CHECK: attributes #6 = { optsize }
 ; CHECK: attributes #7 = { ssp }

llvm/test/Transforms/FunctionAttrs/readattrs.ll

 ; FNATTRS-NEXT:    [[CALL:%.*]] = call ptr @test8_1(ptr [[P]])
 ; FNATTRS-NEXT:    store i32 10, ptr [[CALL]], align 4
 ; FNATTRS-NEXT:    ret void
 ;
 ; ATTRIBUTOR: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(write)
 ; ATTRIBUTOR-LABEL: define {{[^@]+}}@test8_2
 ; ATTRIBUTOR-SAME: (ptr nocapture nofree writeonly [[P:%.*]]) #[[ATTR0]] {
 ; ATTRIBUTOR-NEXT:  entry:
-; ATTRIBUTOR-NEXT:    [[CALL:%.*]] = call ptr @test8_1(ptr nofree readnone [[P]]) #[[ATTR13:[0-9]+]]
+; ATTRIBUTOR-NEXT:    [[CALL:%.*]] = call ptr @test8_1(ptr nofree readnone [[P]]) #[[ATTR14:[0-9]+]]
 ; ATTRIBUTOR-NEXT:    store i32 10, ptr [[CALL]], align 4
 ; ATTRIBUTOR-NEXT:    ret void
 ;
 ; ATTRIBUTOR-CGSCC: Function Attrs: mustprogress nofree nosync nounwind willreturn memory(write)
 ; ATTRIBUTOR-CGSCC-LABEL: define {{[^@]+}}@test8_2
 ; ATTRIBUTOR-CGSCC-SAME: (ptr nofree writeonly [[P:%.*]]) #[[ATTR5:[0-9]+]] {
 ; ATTRIBUTOR-CGSCC-NEXT:  entry:
-; ATTRIBUTOR-CGSCC-NEXT:    [[CALL:%.*]] = call ptr @test8_1(ptr nofree readnone [[P]]) #[[ATTR13:[0-9]+]]
+; ATTRIBUTOR-CGSCC-NEXT:    [[CALL:%.*]] = call ptr @test8_1(ptr nofree readnone [[P]]) #[[ATTR14:[0-9]+]]
 ; ATTRIBUTOR-CGSCC-NEXT:    store i32 10, ptr [[CALL]], align 4
 ; ATTRIBUTOR-CGSCC-NEXT:    ret void
 ;
 entry:
   %call = call ptr @test8_1(ptr %p)
   store i32 10, ptr %call, align 4
   ret void
 }
 
 declare void @llvm.masked.scatter.v4i32.v4p0(<4 x i32>%val, <4 x ptr>, i32, <4 x i1>)
 
 define void @test9(<4 x ptr> %ptrs, <4 x i32>%val) {
 ; FNATTRS: Function Attrs: mustprogress nofree nosync nounwind willreturn memory(write)
 ; FNATTRS-LABEL: define {{[^@]+}}@test9
 ; FNATTRS-SAME: (<4 x ptr> [[PTRS:%.*]], <4 x i32> [[VAL:%.*]]) #[[ATTR7:[0-9]+]] {
 ; FNATTRS-NEXT:    call void @llvm.masked.scatter.v4i32.v4p0(<4 x i32> [[VAL]], <4 x ptr> [[PTRS]], i32 4, <4 x i1> <i1 true, i1 false, i1 true, i1 false>)
 ; FNATTRS-NEXT:    ret void
 ;
 ; ATTRIBUTOR: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(write)
 ; ATTRIBUTOR-LABEL: define {{[^@]+}}@test9
 ; ATTRIBUTOR-SAME: (<4 x ptr> [[PTRS:%.*]], <4 x i32> [[VAL:%.*]]) #[[ATTR0]] {
-; ATTRIBUTOR-NEXT:    call void @llvm.masked.scatter.v4i32.v4p0(<4 x i32> [[VAL]], <4 x ptr> [[PTRS]], i32 4, <4 x i1> <i1 true, i1 false, i1 true, i1 false>) #[[ATTR14:[0-9]+]]
+; ATTRIBUTOR-NEXT:    call void @llvm.masked.scatter.v4i32.v4p0(<4 x i32> [[VAL]], <4 x ptr> [[PTRS]], i32 4, <4 x i1> <i1 true, i1 false, i1 true, i1 false>) #[[ATTR15:[0-9]+]]
 ; ATTRIBUTOR-NEXT:    ret void
 ;
 ; ATTRIBUTOR-CGSCC: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(write)
 ; ATTRIBUTOR-CGSCC-LABEL: define {{[^@]+}}@test9
 ; ATTRIBUTOR-CGSCC-SAME: (<4 x ptr> [[PTRS:%.*]], <4 x i32> [[VAL:%.*]]) #[[ATTR0]] {
-; ATTRIBUTOR-CGSCC-NEXT:    call void @llvm.masked.scatter.v4i32.v4p0(<4 x i32> [[VAL]], <4 x ptr> [[PTRS]], i32 4, <4 x i1> <i1 true, i1 false, i1 true, i1 false>) #[[ATTR14:[0-9]+]]
+; ATTRIBUTOR-CGSCC-NEXT:    call void @llvm.masked.scatter.v4i32.v4p0(<4 x i32> [[VAL]], <4 x ptr> [[PTRS]], i32 4, <4 x i1> <i1 true, i1 false, i1 true, i1 false>) #[[ATTR15:[0-9]+]]
 ; ATTRIBUTOR-CGSCC-NEXT:    ret void
 ;
   call void @llvm.masked.scatter.v4i32.v4p0(<4 x i32>%val, <4 x ptr> %ptrs, i32 4, <4 x i1><i1 true, i1 false, i1 true, i1 false>)
   ret void
 }
 
 declare <4 x i32> @llvm.masked.gather.v4i32.v4p0(<4 x ptr>, i32, <4 x i1>, <4 x i32>)
 define <4 x i32> @test10(<4 x ptr> %ptrs) {
 ; FNATTRS: Function Attrs: mustprogress nofree nosync nounwind willreturn memory(read)
 ; FNATTRS-LABEL: define {{[^@]+}}@test10
 ; FNATTRS-SAME: (<4 x ptr> [[PTRS:%.*]]) #[[ATTR9:[0-9]+]] {
 ; FNATTRS-NEXT:    [[RES:%.*]] = call <4 x i32> @llvm.masked.gather.v4i32.v4p0(<4 x ptr> [[PTRS]], i32 4, <4 x i1> <i1 true, i1 false, i1 true, i1 false>, <4 x i32> undef)
 ; FNATTRS-NEXT:    ret <4 x i32> [[RES]]
 ;
 ; ATTRIBUTOR: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read)
 ; ATTRIBUTOR-LABEL: define {{[^@]+}}@test10
 ; ATTRIBUTOR-SAME: (<4 x ptr> [[PTRS:%.*]]) #[[ATTR7:[0-9]+]] {
-; ATTRIBUTOR-NEXT:    [[RES:%.*]] = call <4 x i32> @llvm.masked.gather.v4i32.v4p0(<4 x ptr> [[PTRS]], i32 4, <4 x i1> <i1 true, i1 false, i1 true, i1 false>, <4 x i32> undef) #[[ATTR15:[0-9]+]]
+; ATTRIBUTOR-NEXT:    [[RES:%.*]] = call <4 x i32> @llvm.masked.gather.v4i32.v4p0(<4 x ptr> [[PTRS]], i32 4, <4 x i1> <i1 true, i1 false, i1 true, i1 false>, <4 x i32> undef) #[[ATTR16:[0-9]+]]
 ; ATTRIBUTOR-NEXT:    ret <4 x i32> [[RES]]
 ;
 ; ATTRIBUTOR-CGSCC: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read)
 ; ATTRIBUTOR-CGSCC-LABEL: define {{[^@]+}}@test10
 ; ATTRIBUTOR-CGSCC-SAME: (<4 x ptr> [[PTRS:%.*]]) #[[ATTR8:[0-9]+]] {
-; ATTRIBUTOR-CGSCC-NEXT:    [[RES:%.*]] = call <4 x i32> @llvm.masked.gather.v4i32.v4p0(<4 x ptr> [[PTRS]], i32 4, <4 x i1> <i1 true, i1 false, i1 true, i1 false>, <4 x i32> undef) #[[ATTR15:[0-9]+]]
+; ATTRIBUTOR-CGSCC-NEXT:    [[RES:%.*]] = call <4 x i32> @llvm.masked.gather.v4i32.v4p0(<4 x ptr> [[PTRS]], i32 4, <4 x i1> <i1 true, i1 false, i1 true, i1 false>, <4 x i32> undef) #[[ATTR16:[0-9]+]]
 ; ATTRIBUTOR-CGSCC-NEXT:    ret <4 x i32> [[RES]]
 ;
   %res = call <4 x i32> @llvm.masked.gather.v4i32.v4p0(<4 x ptr> %ptrs, i32 4, <4 x i1><i1 true, i1 false, i1 true, i1 false>, <4 x i32>undef)
   ret <4 x i32> %res
 }
 
 declare <4 x i32> @test11_1(<4 x ptr>) argmemonly nounwind readonly
 define <4 x i32> @test11_2(<4 x ptr> %ptrs) {

 ; ATTRIBUTOR-CGSCC-LABEL: define {{[^@]+}}@op_bundle_readonly_unknown
 ; ATTRIBUTOR-CGSCC-SAME: (ptr nocapture nofree [[P:%.*]]) {
 ; ATTRIBUTOR-CGSCC-NEXT:    call void @readonly_param(ptr nocapture nofree [[P]]) [ "unknown"() ]
 ; ATTRIBUTOR-CGSCC-NEXT:    ret void
 ;
   call void @readonly_param(ptr %p) ["unknown"()]
   ret void
 }
+
+define i32 @writable_readonly(ptr writable dereferenceable(4) %p) {
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: read)
+; FNATTRS-LABEL: define {{[^@]+}}@writable_readonly
+; FNATTRS-SAME: (ptr nocapture readonly dereferenceable(4) [[P:%.*]]) #[[ATTR15:[0-9]+]] {
+; FNATTRS-NEXT:    [[V:%.*]] = load i32, ptr [[P]], align 4
+; FNATTRS-NEXT:    ret i32 [[V]]
+;
+; ATTRIBUTOR: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: read)
+; ATTRIBUTOR-LABEL: define {{[^@]+}}@writable_readonly
+; ATTRIBUTOR-SAME: (ptr nocapture nofree nonnull readonly dereferenceable(4) [[P:%.*]]) #[[ATTR13:[0-9]+]] {
+; ATTRIBUTOR-NEXT:    [[V:%.*]] = load i32, ptr [[P]], align 4
+; ATTRIBUTOR-NEXT:    ret i32 [[V]]
+;
+; ATTRIBUTOR-CGSCC: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: read)
+; ATTRIBUTOR-CGSCC-LABEL: define {{[^@]+}}@writable_readonly
+; ATTRIBUTOR-CGSCC-SAME: (ptr nocapture nofree nonnull readonly dereferenceable(4) [[P:%.*]]) #[[ATTR13:[0-9]+]] {
+; ATTRIBUTOR-CGSCC-NEXT:    [[V:%.*]] = load i32, ptr [[P]], align 4
+; ATTRIBUTOR-CGSCC-NEXT:    ret i32 [[V]]
+;
+  %v = load i32, ptr %p
+  ret i32 %v
+}
+
+define void @writable_readnone(ptr writable dereferenceable(4) %p) {
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
+; FNATTRS-LABEL: define {{[^@]+}}@writable_readnone
+; FNATTRS-SAME: (ptr nocapture readnone dereferenceable(4) [[P:%.*]]) #[[ATTR1]] {
+; FNATTRS-NEXT:    ret void
+;
+; ATTRIBUTOR: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
+; ATTRIBUTOR-LABEL: define {{[^@]+}}@writable_readnone
+; ATTRIBUTOR-SAME: (ptr nocapture nofree nonnull readnone dereferenceable(4) [[P:%.*]]) #[[ATTR1]] {
+; ATTRIBUTOR-NEXT:    ret void
+;
+; ATTRIBUTOR-CGSCC: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
+; ATTRIBUTOR-CGSCC-LABEL: define {{[^@]+}}@writable_readnone
+; ATTRIBUTOR-CGSCC-SAME: (ptr nocapture nofree nonnull readnone dereferenceable(4) [[P:%.*]]) #[[ATTR1]] {
+; ATTRIBUTOR-CGSCC-NEXT:    ret void
+;
+  ret void
+}
+
 ;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
 ; COMMON: {{.*}}

llvm/test/Transforms/LICM/scalar-promote.ll

 
 loop.1.latch:
   br i1 %c, label %loop.1.header, label %exit
 
 exit:
   ret void
 }
 
-; TODO: The store can be promoted, as sret memory is writable.
-define void @sret_cond_store(ptr sret(i32) noalias %ptr) {
-; CHECK-LABEL: @sret_cond_store(
+define void @cond_store_writable_dereferenceable(ptr noalias writable dereferenceable(4) %ptr) {
+; CHECK-LABEL: @cond_store_writable_dereferenceable(
+; CHECK-NEXT:    [[PTR_PROMOTED:%.*]] = load i32, ptr [[PTR:%.*]], align 4
+; CHECK-NEXT:    br label [[LOOP:%.*]]
+; CHECK:       loop:
+; CHECK-NEXT:    [[V_INC1:%.*]] = phi i32 [ [[V_INC:%.*]], [[LOOP_LATCH:%.*]] ], [ [[PTR_PROMOTED]], [[TMP0:%.*]] ]
+; CHECK-NEXT:    [[C:%.*]] = icmp ult i32 [[V_INC1]], 10
+; CHECK-NEXT:    br i1 [[C]], label [[LOOP_LATCH]], label [[EXIT:%.*]]
+; CHECK:       loop.latch:
+; CHECK-NEXT:    [[V_INC]] = add i32 [[V_INC1]], 1
+; CHECK-NEXT:    br label [[LOOP]]
+; CHECK:       exit:
+; CHECK-NEXT:    [[V_INC1_LCSSA:%.*]] = phi i32 [ [[V_INC1]], [[LOOP]] ]
+; CHECK-NEXT:    store i32 [[V_INC1_LCSSA]], ptr [[PTR]], align 4
+; CHECK-NEXT:    ret void
+;
+  br label %loop
+
+loop:
+  %v = load i32, ptr %ptr
+  %c = icmp ult i32 %v, 10
+  br i1 %c, label %loop.latch, label %exit
+
+loop.latch:
+  %v.inc = add i32 %v, 1
+  store i32 %v.inc, ptr %ptr
+  br label %loop
+
+exit:
+  ret void
+}
+
+define void @cond_store_writable_not_sufficiently_dereferenceable(ptr noalias writable dereferenceable(2) %ptr) {
+; CHECK-LABEL: @cond_store_writable_not_sufficiently_dereferenceable(
 ; CHECK-NEXT:    [[PTR_PROMOTED:%.*]] = load i32, ptr [[PTR:%.*]], align 4
 ; CHECK-NEXT:    br label [[LOOP:%.*]]
 ; CHECK:       loop:
 ; CHECK-NEXT:    [[V_INC1:%.*]] = phi i32 [ [[V_INC:%.*]], [[LOOP_LATCH:%.*]] ], [ [[PTR_PROMOTED]], [[TMP0:%.*]] ]
 ; CHECK-NEXT:    [[C:%.*]] = icmp ult i32 [[V_INC1]], 10
 ; CHECK-NEXT:    br i1 [[C]], label [[LOOP_LATCH]], label [[EXIT:%.*]]
 ; CHECK:       loop.latch:
 ; CHECK-NEXT:    [[V_INC]] = add i32 [[V_INC1]], 1

llvm/test/Verifier/writable-attr.ll

+; RUN: not llvm-as -disable-output %s 2>&1 | FileCheck %s
+
+; CHECK: Attribute 'writable' applied to incompatible type!
+; CHECK-NEXT: ptr @not_pointer
+define void @not_pointer_writable(i32 writable %arg) {
+  ret void
+}
+
+; CHECK: Attributes writable and readnone are incompatible!
+; CHECK-NEXT: ptr @writable_readnone
+define void @writable_readnone(ptr writable readnone %arg) {
+  ret void
+}
+
+; CHECK: Attributes writable and readonly are incompatible!
+; CHECK-NEXT: ptr @writable_readonly
+define void @writable_readonly(ptr writable readonly %arg) {
+  ret void
+}
+
+; CHECK: Attribute writable and memory without argmem: write are incompatible!
+; CHECK-NEXT: ptr @writable_memory_argmem_read
+define void @writable_memory_argmem_read(ptr writable %arg) memory(write, argmem: read) {
+  ret void
+}
+
+; CHECK-NOT: incompatible
+define void @writable_memory_argmem_write(ptr writable %arg) memory(read, argmem: write) {
+  ret void
+}