Add the 'initializes' attribute langref and support #84803

haopliu · 2024-03-11T17:55:20Z

We propose adding a new LLVM attribute, initializes((Lo1,Hi1),(Lo2,Hi2),...), which expresses the notion of memory space (i.e., intervals, in bytes) that the argument pointing to is initialized in the function.

Will commit the attribute inferring in the follow-up PRs.

https://discourse.llvm.org/t/rfc-llvm-new-initialized-parameter-attribute-for-improved-interprocedural-dse/77337

llvmbot · 2024-03-11T21:24:15Z

@llvm/pr-subscribers-llvm-adt
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Haopeng Liu (haopliu)

Changes

We propose adding a new LLVM attribute, initialized((Lo1,Hi1),(Lo2,Hi2),...), which expresses the notion of memory space (i.e., intervals, in bytes) that the argument pointing to is initialized in the function.

Will commit the attribute inferring in the follow-up PRs.

https://discourse.llvm.org/t/rfc-llvm-new-initialized-parameter-attribute-for-improved-interprocedural-dse/77337

Full diff: https://github.com/llvm/llvm-project/pull/84803.diff

1 Files Affected:

(modified) llvm/docs/LangRef.rst (+22)

diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index b70220dec92615..39a555edfa80d6 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -1621,6 +1621,28 @@ Currently, only the following parameter attributes are defined:
     ``readonly`` or a ``memory`` attribute that does not contain
     ``argmem: write``.
 
+``initialized((Lo1,Hi1),...)``
+    This attribute is a list of const ranges in ascending order with no
+    overlapping or continuous. It indicates that the function initializes the
+    memory through the pointer argument, [%p+LoN, %p+HiN): there are no reads,
+    and no special accesses (such as volatile access or untrackable capture)
+    before the initialization in the function. LoN/HiN are 64-bit ints;
+    negative values are allowed in case a pointer to partway through the
+    allocation is passed to.
+
+    This attribute implies that the function initializes and does not read
+    before initialization through this pointer argument, even though it may
+    read the memory before initialization that the pointer points to, such
+    as through other arguments.
+
+    The ``writable`` or ``dereferenceable`` attribute does not imply
+    ``initialized`` attribute, and ``initialized`` does not imply ``writeonly``
+    since cases that read from the pointer after write, can be ``initialized``
+    but not ``writeonly``.
+
+    Note that this attribute does not apply to the unwind edge: the memory may
+    not actually be written to when unwinding happens.
+
 ``dead_on_unwind``
     At a high level, this attribute indicates that the pointer argument is dead
     if the call unwinds, in the sense that the caller will not depend on the

jvoung · 2024-03-12T15:30:04Z

llvm/docs/LangRef.rst

@@ -1621,6 +1621,28 @@ Currently, only the following parameter attributes are defined:
    ``readonly`` or a ``memory`` attribute that does not contain
    ``argmem: write``.

+``initialized((Lo1,Hi1),...)``
+    This attribute is a list of const ranges in ascending order with no
+    overlapping or continuous. It indicates that the function initializes the


nit: "with no overlapping or adjoining list elements"? or something like that (felt like it was missing a word at the end at least)

We should start this off with a one sentence overview of the attribute. So starting with something like "This indicates that the function initializes the ranges of the pointer parameter's memory." Then describe what "initialize" means. Then the random details like non-overlapping/continuous ranges at the end.

jvoung · 2024-03-12T15:31:43Z

llvm/docs/LangRef.rst

+    and no special accesses (such as volatile access or untrackable capture)
+    before the initialization in the function. LoN/HiN are 64-bit ints;
+    negative values are allowed in case a pointer to partway through the
+    allocation is passed to.


nit: "in case the argument points partway into an allocation." ?

jvoung · 2024-03-12T15:36:08Z

llvm/docs/LangRef.rst

+    as through other arguments.
+
+    The ``writable`` or ``dereferenceable`` attribute does not imply
+    ``initialized`` attribute, and ``initialized`` does not imply ``writeonly``


nit: "does not imply [the] initialized attribute, and ... writeonly, since initialized allows reading from the pointer after writing." ?

aeubanks · 2024-03-12T19:59:08Z

we should combine this PR with the PR that adds support for this in LLVM, or else it's weird if we're documenting something that LLVM doesn't support yet

aeubanks · 2024-03-12T20:04:11Z

llvm/docs/LangRef.rst

+    since cases that read from the pointer after write, can be ``initialized``
+    but not ``writeonly``.
+
+    Note that this attribute does not apply to the unwind edge: the memory may


as mentioned in the RFC, we should be more accurate here. the part of the attribute where the memory is read from before a write still must apply on the unwind edge. Also, I'd move this up since this is important to the semantics

sorry, I meant that the memory is not read from before a write

aeubanks · 2024-03-13T20:39:31Z

unintentional add of FunctionAttrs.cpp change?

we should combine this PR with the PR that adds support for this in LLVM, or else it's weird if we're documenting something that LLVM doesn't support yet

specifically just the attribute support (e.g. bitcode) not any of the inference or usage of it

llvm/docs/LangRef.rst

aeubanks · 2024-03-13T20:59:01Z

llvm/docs/LangRef.rst

+    special accesses (such as volatile access or untrackable capture) before
+    the initialization write in the function.
+
+    This attribute implies that the function initializes and does not read


I wouldn't use implies.

`This attribute only holds for the memory accessed via this pointer parameter. Other arbitrary accesses to the same memory via other pointers are allowed.

aeubanks · 2024-03-13T21:00:32Z

llvm/docs/LangRef.rst

+    written to when unwinding happens.
+
+    The ``writable`` or ``dereferenceable`` attribute does not imply the
+    ``initialized`` attribute, and ``initialized`` does not imply ``writeonly``


I'd separate this into two sentences.

aeubanks · 2024-03-13T21:00:43Z

llvm/docs/LangRef.rst

+    ``initialized`` attribute, and ``initialized`` does not imply ``writeonly``
+    since ``initialized`` allows reading from the pointer after writing.
+
+    This attribute is a list of const ranges in ascending order with no


s/const/constant

aeubanks · 2024-03-13T21:01:25Z

llvm/docs/LangRef.rst

+    since ``initialized`` allows reading from the pointer after writing.
+
+    This attribute is a list of const ranges in ascending order with no
+    overlapping or adjoining list elements. LoN/HiN are 64-bit ints, and


I haven't heard "adjoining" used in this context, I think "consecutive" is more commonly used.

aeubanks · 2024-03-13T21:01:47Z

llvm/docs/LangRef.rst

+    since ``initialized`` allows reading from the pointer after writing.
+
+    This attribute is a list of const ranges in ascending order with no
+    overlapping or adjoining list elements. LoN/HiN are 64-bit ints, and


put LoN/HiN in double backticks

nikic · 2024-03-13T21:55:29Z

As a heads up, the inference implementation for this (or at least the available prototype) does not look viable from a compile-time perspective: https://llvm-compile-time-tracker.com/compare.php?from=69b09b43b0a2057918078edb401adab888d1014b&to=0bd68ae2d56783377acc0aa5d7958b47411b8342&stat=instructions:u

…oring.

haopliu · 2024-03-14T17:11:08Z

As a heads up, the inference implementation for this (or at least the available prototype) does not look viable from a compile-time perspective: https://llvm-compile-time-tracker.com/compare.php?from=69b09b43b0a2057918078edb401adab888d1014b&to=0bd68ae2d56783377acc0aa5d7958b47411b8342&stat=instructions:u

Thanks for this heads up! This PR only adds the attribute support. Will further tune the inferring performance and post the updated compile time tracker in the inference PR.

haopliu · 2024-03-14T17:12:19Z

Thank you all! Update the LangRef, and add the attribute support. Please take another look :-D

llvm/include/llvm/ADT/FoldingSet.h

jvoung · 2024-03-14T20:24:23Z

llvm/include/llvm/IR/Attributes.td

@@ -38,6 +38,9 @@ class IntAttr<string S, list<AttrProperty> P> : Attr<S, P>;
 /// Type attribute.
 class TypeAttr<string S, list<AttrProperty> P> : Attr<S, P>;

+/// Const range list attribute.


nit: put closer to the ConstantRangeAttr?

jvoung · 2024-03-14T20:26:39Z

llvm/include/llvm/IR/Attributes.td

@@ -318,6 +321,9 @@ def Writable : EnumAttr<"writable", [ParamAttr]>;
 /// Function only writes to memory.
 def WriteOnly : EnumAttr<"writeonly", [ParamAttr]>;

+/// Pointer argument memory [%p+LoN, %p+HiN) is initialized.


The list looks almost sorted, so may be better to put it near InAlloca or ?

jvoung · 2024-03-14T20:29:27Z

llvm/include/llvm/AsmParser/LLParser.h

@@ -307,6 +308,10 @@ namespace llvm {
                                bool AllowParens = false);
    bool parseOptionalCodeModel(CodeModel::Model &model);
    bool parseOptionalDerefAttrBytes(lltok::Kind AttrKind, uint64_t &Bytes);
+    bool parseConstRange(std::pair<int64_t, int64_t> &Range);
+    bool parseInitializedRanges(
+        lltok::Kind AttrKind,


can the AttrKind be hardcoded within the function instead of taking as parameter? (unlike DerefAttrBytes, there is only one variation of the attribute, I believe?)

jvoung · 2024-03-14T20:33:13Z

llvm/include/llvm/IR/Attributes.h

@@ -94,6 +94,8 @@ class Attribute {

  static const unsigned NumIntAttrKinds = LastIntAttr - FirstIntAttr + 1;
  static const unsigned NumTypeAttrKinds = LastTypeAttr - FirstTypeAttr + 1;
+  static const unsigned NumConstRangeListAttrKinds =


Is this used by anything (if not, can remove)? I don't see the ConstantRangeAttrKind version of it, and hard to see "NumTypeAttrKinds" used

jvoung · 2024-03-14T20:53:18Z

llvm/include/llvm/IR/Attributes.h

@@ -186,6 +193,9 @@ class Attribute {
  /// Return true if the attribute is a type attribute.
  bool isTypeAttribute() const;

+  /// Return true if the attribute is a const range list attribute.
+  bool isConstRangeListAttribute() const;


nit: wonder if should s/Const/Constant/ to be consistent with isConstantRangeAttribute, etc.

+1, no need to abbreviate

jvoung · 2024-03-14T23:58:52Z

llvm/include/llvm/ADT/FoldingSet.h

@@ -362,6 +362,21 @@ class FoldingSetNodeID {
    }
  }

+  void AddRanges(const SmallVector<std::pair<int64_t, int64_t>, 16> &Ranges) {


Is it possible to use ArrayRef or something to not need specific SmallVector inline size of 16 here? "Prefer to use ArrayRef or SmallVectorImpl as a parameter type." under https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h

jvoung · 2024-03-15T00:01:23Z

llvm/include/llvm/IR/Attributes.h

@@ -226,6 +236,10 @@ class Attribute {
  /// attribute to be a ConstantRange attribute.
  ConstantRange getValueAsConstantRange() const;

+  /// Return the attribute's value as a const range list. This requires the
+  /// attribute to be a const range list attribute.
+  SmallVector<std::pair<int64_t, int64_t>, 16> getValueAsRanges() const;


Is it possible to return an ArrayRef instead of a copy? Will the underlying storage lifetime work out?

jvoung · 2024-03-15T00:06:34Z

llvm/lib/AsmParser/LLParser.cpp

+  if (parseInt64(Range.first))
+    return true;
+
+  if (EatIfPresent(lltok::comma)) {


should it be an error to be missing the comma?

jvoung · 2024-03-15T00:15:52Z

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

@@ -930,11 +932,24 @@ void ModuleBitcodeWriter::writeAttributeGroupTable() {
        Record.push_back(getAttrKindEncoding(Attr.getKindAsEnum()));
        if (Ty)
          Record.push_back(VE.getTypeID(Attr.getValueAsType()));
-      } else {
+      } else if (Attr.isConstantRangeAttribute()) {
        assert(Attr.isConstantRangeAttribute());


nit: can remove the assert?

jvoung · 2024-03-15T00:25:10Z

llvm/lib/IR/Verifier.cpp

+    Check(!Inits.empty(), "Attribute 'initialized' does not support empty list",
+          V);
+
+    for (size_t i = 1; i < Inits.size(); i++) {


perhaps check low < high for the same "i"?

aeubanks · 2024-03-14T23:06:08Z

llvm/include/llvm/IR/Attributes.h

@@ -226,6 +236,10 @@ class Attribute {
  /// attribute to be a ConstantRange attribute.
  ConstantRange getValueAsConstantRange() const;

+  /// Return the attribute's value as a const range list. This requires the
+  /// attribute to be a const range list attribute.
+  SmallVector<std::pair<int64_t, int64_t>, 16> getValueAsRanges() const;


we should use ConstantRange instead of std::pair, especially since it already has intersection/union methods

also, 16 elements for the small vector optimization seems very high, I feel the vast majority of cases will be at most 4. I'd choose 4 (or even 2)

aeubanks · 2024-03-14T23:10:54Z

llvm/lib/IR/Attributes.cpp

@@ -616,6 +652,23 @@ std::string Attribute::getAsString(bool InAttrGrp) const {
    return Result;
  }

+  if (hasAttribute(Attribute::Initialized)) {
+    auto Ranges = getValueAsRanges();
+    if (Ranges.empty())


is this allowed? I think we should forbid an empty list

oh I see the verifier check below, so I don't think we need this check

aeubanks · 2024-03-14T23:12:31Z

llvm/lib/IR/Verifier.cpp

@@ -1993,6 +1993,14 @@ void Verifier::verifyParameterAttrs(AttributeSet Attrs, Type *Ty,
          Attrs.hasAttribute(Attribute::ReadOnly)),
        "Attributes writable and readonly are incompatible!", V);

+  Check(!(Attrs.hasAttribute(Attribute::Initialized) &&


I don't think this is true. A function that immediately unwinds can be marked as initializing all arguments according to our semantics, but is also readnone

llvm/docs/LangRef.rst

aeubanks · 2024-03-15T19:52:30Z

llvm/include/llvm/IR/Attributes.h

@@ -186,6 +193,9 @@ class Attribute {
  /// Return true if the attribute is a type attribute.
  bool isTypeAttribute() const;

+  /// Return true if the attribute is a const range list attribute.
+  bool isConstRangeListAttribute() const;


+1, no need to abbreviate

aeubanks · 2024-03-15T19:54:22Z

llvm/docs/LangRef.rst

+    parameter. Other arbitrary accesses to the same memory via other pointers
+    are allowed.
+
+    Note that this attribute does not apply to the unwind edge: the


this is now redundant with the unwind comment above

nikic · 2024-03-15T21:30:10Z

As a heads up, the inference implementation for this (or at least the available prototype) does not look viable from a compile-time perspective: https://llvm-compile-time-tracker.com/compare.php?from=69b09b43b0a2057918078edb401adab888d1014b&to=0bd68ae2d56783377acc0aa5d7958b47411b8342&stat=instructions:u

Thanks for this heads up! This PR only adds the attribute support. Will further tune the inferring performance and post the updated compile time tracker in the inference PR.

The main thing I would be concerned about at this point is whether the entire approach is viable or not -- I'd rather not add the attribute (plus a whole new attribute kind) if it later turns out that this is too expensive.

Anyway, a couple of high level comments:

I'm not sure this is the ideal attribute name. initialized to me sounds like this attribute promises that the memory is initialized on entry to the function, not that the function will initialize it. Maybe will_initialize is a better name? Or initializes?
How important is it to represent multiple ranges? We have just added support for ConstantRange attributes, so a single range can be represented out of the box. This also avoids the tricky question of how to deal with non-zero start offsets in DSE, which we don't really have AA support for right now. My intuition would be that you usually get whole object initializations -- but maybe this is not the case due to padding holes?
Failing that, I feel like we should still piggy-back more on the new ConstantRange attribute kind. We could convert that into a list of ConstantRange stored as TrailingObjects and make the current single-range case a special case of the general multiple-range case.

aeubanks · 2024-03-25T21:29:30Z

The main thing I would be concerned about at this point is whether the entire approach is viable or not -- I'd rather not add the attribute (plus a whole new attribute kind) if it later turns out that this is too expensive.

+1

Anyway, a couple of high level comments:

I'm not sure this is the ideal attribute name. initialized to me sounds like this attribute promises that the memory is initialized on entry to the function, not that the function will initialize it. Maybe will_initialize is a better name? Or initializes?

Makes sense, initializes sounds good.

How important is it to represent multiple ranges? We have just added support for ConstantRange attributes, so a single range can be represented out of the box. This also avoids the tricky question of how to deal with non-zero start offsets in DSE, which we don't really have AA support for right now. My intuition would be that you usually get whole object initializations -- but maybe this is not the case due to padding holes?

Yes the concern was padding holes. @haopliu do you have any data on how often padding prevents whole object initialization?

aeubanks · 2024-03-25T21:43:33Z

Even if we represent padding more precisely with multiple ranges in initializes, clang generates a memset of the entire alloca and DSE can only remove the part of the memset that corresponds to the first range and the last range, since it doesn't split memsets. So if we had one padding hole and used multiple ranges to represent everything else, we'd still end up with a store to the padding hole.

I think !tbaa.struct was designed for this, but we don't emit it for -ftrivial-auto-var-init. Also it might not apply to memset right now, only memcpy?

aeubanks · 2024-03-25T22:13:16Z

godbolt seems to not show metadata (I'm not seeing it on a memcpy on godbolt but am seeing it locally), but my point still stands

aeubanks · 2024-03-26T23:01:54Z

actually I'm unsure if it would be legal to remove stores on padding bytes

haopliu · 2024-04-25T21:19:40Z

A quick question before I look into this in more detail: Why do we need the dedicated ConstantRangeList class? I'd have expected to see this passed around as ArrayRef<ConstantRange> and stored as TrailingObjects<ConstantRange>.

I see it has some helpers to insert ranges in sorted order, but that seems more like something for the inference implementation rather than the attribute implementation.

Changed the attribute implementation to TrailingObjects<ConstantRange> + ArrayRef<ConstantRange>, and meanwhile kept the helper class ConstantRangeList for intersectWith/unionWith/... in FunctionAttrs.cpp and DSE.cpp.

Please take another look. Thanks!

jvoung · 2024-04-26T13:27:19Z

llvm/lib/IR/LLVMContextImpl.h

@@ -1565,6 +1566,8 @@ class LLVMContextImpl {
  UniqueStringSaver Saver{Alloc};
  SpecificBumpPtrAllocator<ConstantRangeAttributeImpl>
      ConstantRangeAttributeAlloc;
+  SpecificBumpPtrAllocator<ConstantRangeListAttributeImpl>


It seems this isn't used anymore? Just using Alloc now?

Then the forward declaration of ConstantRangeListAttributeImpl above isn't needed.

jvoung · 2024-04-26T13:30:12Z

llvm/lib/IR/AttributeImpl.h

+      private TrailingObjects<ConstantRangeListAttributeImpl, ConstantRange> {
+  friend TrailingObjects;
+
+  unsigned ValSize;


nit: maybe just "Size" since there isn't a field named "Val"

jvoung · 2024-04-26T13:35:56Z

llvm/lib/IR/ConstantRangeList.cpp

+    Ranges.insert(Ranges.begin(), NewRange);
+    return;
+  }
+  if (std::find(Ranges.begin(), Ranges.end(), NewRange) != Ranges.end()) {


I think what Arthur meant (correct me if I'm wrong), is remove this std::find. The std::lower_bound below already does a "find" in the list.

Then check if LowerBound == NewRange. If so, then we've found a duplicate and can early return the way that this std::find() had previously done. I think that's what he meant by "check just one element" (the LowerBound element).

So then, also maybe you don't need the special case size() == 1 with overlap (not sure that's common).

haopliu · 2024-05-06T20:40:30Z

Here is an updated compile time regression of the entire stack of patches:
this current PR + attribute inference with a dataflow approach (53bd30) + apply the attr in DSE (d8d842).
https://llvm-compile-time-tracker.com/compare.php?from=07d7b9c255078edc6f04bd4e68416bdf3e8735ab&to=d8d8420746ad988173adf5d71e9d51cd8d3975e7&stat=instructions%3Au

aeubanks

lgtm with one last nit

I'd like some other approvals before landing this

for the followup patches, the inference patch generally lgtm (since I wrote a lot of it :) ), and the DSE patch in principle makes sense (the important thing is that it allows an instruction to be associated with multiple MemoryDefs), although I still need to understand it better. 0.07% compile time regression for interprocedural DSE seems acceptable IMO

aeubanks · 2024-05-06T20:34:09Z

llvm/lib/IR/ConstantRangeList.cpp

+  }
+}
+
+void ConstantRangeList::print(raw_ostream &OS) const {


doesn't really matter, they're the same

aeubanks · 2024-05-06T20:34:16Z

llvm/lib/IR/ConstantRangeList.cpp

+
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+LLVM_DUMP_METHOD void ConstantRangeList::dump() const {
+  print(llvm::dbgs());


remove llvm::

jvoung

Nice work reducing the compile time from the initial version =)

jvoung · 2024-05-08T01:38:27Z

llvm/include/llvm/IR/Attributes.h

@@ -267,6 +281,9 @@ class Attribute {
  /// Returns the value of the range attribute.
  ConstantRange getRange() const;

+  /// Returns the value of the initializes attribute.
+  ConstantRangeList getInitializes() const;


nit: Perhaps return ArrayRef to be consistent with getValueAsConstantRangeList(), then the caller can decide to make it a ConstantRangeList or not?

Also ArrayRef allows the caller to avoid a copy if it isn't needed.

nikic · 2024-05-09T01:40:26Z

llvm/docs/LangRef.rst

+    parameter. Other arbitrary accesses to the same memory via other pointers
+    are allowed.
+
+    The ``writable`` or ``dereferenceable`` attribute does not imply the


Suggested change

The ``writable`` or ``dereferenceable`` attribute does not imply the

The ``writable`` or ``dereferenceable`` attribute do not imply the

nikic · 2024-05-09T01:40:42Z

llvm/docs/LangRef.rst

+    are allowed.
+
+    The ``writable`` or ``dereferenceable`` attribute does not imply the
+    ``initializes`` attribute. The ``initializes`` does not imply ``writeonly``


Suggested change

``initializes`` attribute. The ``initializes`` does not imply ``writeonly``

``initializes`` attribute. The ``initializes`` attribute does not imply ``writeonly``

nikic · 2024-05-09T01:42:00Z

llvm/docs/LangRef.rst

+    since ``initializes`` allows reading from the pointer after writing.
+
+    This attribute is a list of constant ranges in ascending order with no
+    overlapping or consecutive list elements. ``LoN/HiN`` are 64-bit ints, and


Suggested change

overlapping or consecutive list elements. ``LoN/HiN`` are 64-bit ints, and

overlapping or consecutive list elements. ``LoN/HiN`` are 64-bit integers, and

Though I wonder whether the integers shouldn't match the pointer index type rather than being hardcoded to 64-bits...

nikic · 2024-05-09T01:42:59Z

llvm/include/llvm/IR/ConstantRangeList.h

@@ -0,0 +1,86 @@
+//===- ConstantRangeList.h - A list of constant range -----------*- C++ -*-===//


Suggested change

//===- ConstantRangeList.h - A list of constant range -----------*- C++ -*-===//

//===- ConstantRangeList.h - A list of constant ranges -----------*- C++ -*-===//

nikic · 2024-05-09T01:43:22Z

llvm/include/llvm/IR/ConstantRangeList.h

+//===----------------------------------------------------------------------===//
+//
+// Represent a list of signed ConstantRange and do NOT support wrap around the
+// end of the numeric range. Ranges in the list are ordered and no overlapping.


Suggested change

// end of the numeric range. Ranges in the list are ordered and no overlapping.

// end of the numeric range. Ranges in the list are ordered and not overlapping.

or "non-overlapping".

nikic · 2024-05-09T01:47:28Z

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

+
+          SmallVector<ConstantRange, 2> Val;
+          int RangeSize = Record[++i];
+          assert(i + 2 * RangeSize < e);


This can't be an assert, it should be an if with error return.

Ah, changed to an if with error return!

nikic · 2024-05-09T01:49:20Z

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

+        Record.push_back(Val.size());
+        for (auto &CR : Val) {
+          emitSignedInt64(Record, CR.getLower().getSExtValue());
+          emitSignedInt64(Record, CR.getUpper().getSExtValue());


This hardcodes the 64-bit ranges in the bitcode format. We shouldn't do that to allow future uses of this attribute type with different width. This should use emitConstantRange() instead, maybe after a small refactoring to allow emitting the bit width separately (as we don't need to emit it for each range...)

Ah I wasn't imagining that a future attribute might want to use ConstantRangeList, but that does seem like something we should support.

Changed to use emitConstantRange().

nikic · 2024-05-09T01:52:01Z

llvm/lib/IR/Attributes.cpp

+        ConstantRangeListAttributeImpl::totalSizeToAlloc(Val),
+        alignof(ConstantRangeListAttributeImpl));
+    PA = new (Mem) ConstantRangeListAttributeImpl(Kind, Val);
+    pImpl->AttrsSet.InsertNode(PA, InsertPoint);


I guess this ends up working fine right now where the ConstantRanges have <= 64 bits, but in general I think this may result in a memory leak due to the dtor not running.

How to call the dtor explicitly?

hmm I'm not seeing where the existing ConstantRangeAttributeImpl's destructor gets called

ah this should be using a separate SpecificBumpPtrAllocator like ConstantRangeAttributeAlloc so that we make sure to call destructors when the LLVMContext is destroyed

oh thanks for this info! Changed to SpecificBumpPtrAllocator. Please take another look :-)

nikic · 2024-05-09T01:52:38Z

llvm/lib/IR/ConstantRangeList.cpp

+// Represent a list of signed ConstantRange and do NOT support wrap around the
+// end of the numeric range. Ranges in the list are ordered and no overlapping.
+// Ranges should have the same bitwidth. Each range's lower should be less than
+// its upper.


Don't repeat the header comment here.

llvm/include/llvm/IR/ConstantRangeList.h

nikic

FWIW, what I originally had in mind was to actually not have separate ConstantRange and ConstantRangeList attribute kinds, and only have the latter -- the existing ConstantRange kind would just be a list with a single element.

The upside of doing that is that we have less attribute kinds to deal with :) The downside is that the representation for the single-attribute case becomes less efficient (because we have to explicitly store the size 1).

aeubanks · 2024-05-09T05:15:01Z

FWIW, what I originally had in mind was to actually not have separate ConstantRange and ConstantRangeList attribute kinds, and only have the latter -- the existing ConstantRange kind would just be a list with a single element.

The upside of doing that is that we have less attribute kinds to deal with :) The downside is that the representation for the single-attribute case becomes less efficient (because we have to explicitly store the size 1).

The less efficient single-range seems ok if the single range attributes aren't too common and have too many unique ranges. It's hard to predict the future to know if this will be the case. Supporting a separate ConstantRangeList attribute doesn't seem too bad though, so I'm leaning toward keeping them separate.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

aeubanks · 2024-05-15T22:38:59Z

llvm/lib/IR/ConstantRangeList.cpp

+  // Slow insert.
+  SmallVector<ConstantRange, 2> ExistingTail(LowerBound, Ranges.end());
+  Ranges.erase(LowerBound, Ranges.end());
+  // "sle" instead of "slt" to merge consecutive ranges.


the "sle" instead of "slt" portion of the comment isn't necessary

Removed this portion!

aeubanks · 2024-05-15T22:41:20Z

llvm/lib/IR/Attributes.cpp

+        ConstantRangeListAttributeImpl::totalSizeToAlloc(Val),
+        alignof(ConstantRangeListAttributeImpl));
+    PA = new (Mem) ConstantRangeListAttributeImpl(Kind, Val);
+    pImpl->AttrsSet.InsertNode(PA, InsertPoint);


hmm I'm not seeing where the existing ConstantRangeAttributeImpl's destructor gets called

…structors

jvoung · 2024-05-17T14:14:09Z

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

+          if (i + 2 * RangeSize >= e)
+            return error("Incomplete constant range list");
+          for (unsigned Idx = 0; Idx < RangeSize; ++Idx) {
+            int64_t Start = BitcodeReader::decodeSignRotatedValue(Record[++i]);


The int64_t as type would still assume BitWidth <= 64 (if > 64, the writer actually writes more than 2 elements in the record per Idx)

Can you factor out part of readConstantRange() and reuse that (the part after having read the BitWidth from the record)?

Refactored readConstantRange and reused it to avoid the bitwidth assumption. Thanks!

aeubanks · 2024-05-17T17:44:57Z

@nikic any more concerns?

aeubanks · 2024-05-28T19:21:30Z

ping @nikic

nikic · 2024-05-29T13:44:56Z

llvm/include/llvm/IR/ConstantRangeList.h

+  bool empty() const { return Ranges.empty(); }
+
+  /// Get the bit width of this ConstantRangeList.
+  uint32_t getBitWidth() const { return 64; }


Hardcoding this seems a bit odd for a generically named class like ConstantRangeList, but I guess this can always be relaxed later...

Yeah, relaxing later on demand is the plan!

nikic · 2024-05-29T14:11:53Z

llvm/include/llvm/IR/ConstantRangeList.h

+// Represent a list of signed ConstantRange and do NOT support wrap around the
+// end of the numeric range. Ranges in the list are ordered and not overlapping.
+// Ranges should have the same bitwidth. Each range's lower should be less than
+// its upper.


Something I noticed is that while ConstantRangeList maintains the "ordered and not overlapping" invariant through manipulations, it does not check that it initially holds. And indeed, LLParser will create such invalid ConstantRangeLists, which will later get rejected by the Verifier. Is this how it is intended to work out?

it is convenient that the verifier can check for sorted ConstantRangeLists without asserting on construction, but perhaps it is better to move the checks to both LLParser and bitcode parser

a static method that returns a std::optional<ConstantRangeList> depending on if the input is sorted or not, and error checking in the LLParser/bitcode parser would be good. then we can assert that the input is sorted in the constructor.

(or just automatically sort ConstantRangeLists on construction, but IMO the checks can be useful for finding bugs)

Added static std::optional<ConstantRangeList> getConstantRangeList() and checked the optional return in LLParse and bitcode parser.

nikic · 2024-05-29T14:15:28Z

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

+
+          SmallVector<ConstantRange, 2> Val;
+          unsigned RangeSize = Record[++i];
+          unsigned BitWidth = Record[++i];


Missing bounds check before these accesses.

nikic · 2024-05-29T14:17:13Z

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

+          if (i + 2 * RangeSize >= e)
+            return error("Incomplete constant range list");
+          for (unsigned Idx = 0; Idx < RangeSize; ++Idx) {
+            i++;


I don't really get why this i++ and i-- dance here is necessary. I think maybe you want just one ++i before the loop?

Moved to before the loop and changed the Record access to "i++" rather than "++i", the same way as the "Range" attribute.

nikic · 2024-05-29T14:32:04Z

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

+        Record.push_back(Val[0].getBitWidth());
+        for (auto &CR : Val) {
+          emitConstantRange(Record, CR, /*EmitBitWidth=*/false);
+        }


Remove braces.

llvm/lib/IR/AttributeImpl.h

nikic · 2024-05-29T14:34:45Z

llvm/lib/IR/AttributeImpl.h

+    assert(Size > 0);
+    unsigned BitWidth = Val.front().getLower().getBitWidth();
+    for (unsigned I = 0; I != Size; ++I) {
+      assert(BitWidth == Val[I].getLower().getBitWidth());


Suggested change

assert(BitWidth == Val[I].getLower().getBitWidth());

assert(BitWidth == Val[I].getBitWidth());

Removed this code snippet.

nikic · 2024-05-29T14:36:35Z

llvm/lib/IR/AttributeImpl.h

+    unsigned BitWidth = Val.front().getLower().getBitWidth();
+    for (unsigned I = 0; I != Size; ++I) {
+      assert(BitWidth == Val[I].getLower().getBitWidth());
+      new (&TrailingCR[I]) ConstantRange(BitWidth, false);


Can't you use uninitialized_copy instead of performing a dummy initialization first?

Removed this code snippet.

nikic · 2024-05-29T14:41:24Z

llvm/lib/IR/Attributes.cpp

+    // If we didn't find any existing attributes of the same shape then create a
+    // new one and insert it.
+    PA = new (pImpl->ConstantRangeListAttributeAlloc.Allocate(
+        ConstantRangeListAttributeImpl::totalSizeToAlloc(Val)))


Heh, this does something completely different from what you think it does. It allocates an array of sizeof(ConstantRangeListAttributeImpl) * totalSizeToAlloc(Val).

It's not possible to use SpecificBumpPtrAllocator with a dynamically sized class, it can only be used to allocate objects of fixed size.

if we have a variable number of ConstantRanges in a ConstantRangeListAttributeImpl and they need a destructor, I don't see a nice way of using trailing objects and bump allocators. I think we should go back to ConstantRangeListAttributeImpl containing a ConstantRangeList and using SpecificBumpPtrAllocator<ConstantRangeListAttributeImpl> so that we allocate the right amount of memory and properly call the destructor. most of the time we'll be in the non-allocating version of the SmallVector<ConstantRange> anyway

Changed the attr implementation back to ConstantRangeList.

haopliu · 2024-05-30T23:17:19Z

Thanks for the comments, Nikita and Arthur! @nikic please take another look.

Add 'initialized' attribute langref

2554c82

haopliu requested review from aeubanks and efriedma-quic March 11, 2024 17:55

haopliu marked this pull request as ready for review March 11, 2024 21:23

llvmbot added the llvm:ir label Mar 11, 2024

jvoung reviewed Mar 12, 2024

View reviewed changes

aeubanks reviewed Mar 12, 2024

View reviewed changes

haopliu added 2 commits March 12, 2024 22:10

Update the LangRef

a9256bf

Refactor the argument uses collecting to a function

9150ece

llvmbot added the llvm:transforms label Mar 12, 2024

aeubanks reviewed Mar 13, 2024

View reviewed changes

Add 'initialized' attribute support and undo the FunctionAttrs refact…

206b707

…oring.

llvmbot added the llvm:adt label Mar 14, 2024

Update the LangRef (round 2)

eaf5ca0

haopliu changed the title ~~Add 'initialized' attribute langref~~ Add the 'initialized' attribute langref and support Mar 14, 2024

kuhar reviewed Mar 14, 2024

View reviewed changes

llvm/include/llvm/ADT/FoldingSet.h Outdated Show resolved Hide resolved

Update FoldingSet.h

52cc649

jvoung reviewed Mar 15, 2024

View reviewed changes

aeubanks reviewed Mar 15, 2024

View reviewed changes

Change the attr impl to TrailingObjects<ConstantRange>

4eeba08

jvoung reviewed Apr 26, 2024

View reviewed changes

Update LLVMContextImpl.h

502e062

aeubanks approved these changes May 6, 2024

View reviewed changes

jvoung approved these changes May 8, 2024

View reviewed changes

haopliu and others added 4 commits May 8, 2024 10:59

Merge branch 'llvm:main' into dse-commit

fac03b8

Rebase to the latest main

ea3e7c5

Change getInitializes() to return ArrayRef<ConstantRange>

7630510

Remove llvm::

a604b4f

aeubanks requested a review from fhahn May 8, 2024 21:23

nikic reviewed May 9, 2024

View reviewed changes

haopliu added 2 commits May 9, 2024 14:34

Update LangRef and ConstantRangeList

f941b24

Update BitCodeReader/Writer

e52fd9d

aeubanks reviewed May 9, 2024

View reviewed changes

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp Outdated Show resolved Hide resolved

Encode bitwidth once for ConstantRangeList kind attr

8e64fa9

aeubanks reviewed May 15, 2024

View reviewed changes

Use SpecificBumpPtrAllocator to allocate mem and make sure to call de…

9ae0fca

…structors

jvoung reviewed May 17, 2024

View reviewed changes

Refactor readConstantRange and update BitcodeReader

d4809ab

Merge branch 'main' into dse-commit

99d36cc

nikic reviewed May 29, 2024

View reviewed changes

haopliu added 2 commits May 30, 2024 21:54

Change the attr imple back to ConstantRangeList

96191cd

Add getConstantRangeList unittest

645f577

	The ``writable`` or ``dereferenceable`` attribute does not imply the
	The ``writable`` or ``dereferenceable`` attribute do not imply the

	``initializes`` attribute. The ``initializes`` does not imply ``writeonly``
	``initializes`` attribute. The ``initializes`` attribute does not imply ``writeonly``

	overlapping or consecutive list elements. ``LoN/HiN`` are 64-bit ints, and
	overlapping or consecutive list elements. ``LoN/HiN`` are 64-bit integers, and

		@@ -0,0 +1,86 @@
		//===- ConstantRangeList.h - A list of constant range ------------ C++ --===//

	// end of the numeric range. Ranges in the list are ordered and no overlapping.
	// end of the numeric range. Ranges in the list are ordered and not overlapping.

	assert(BitWidth == Val[I].getLower().getBitWidth());
	assert(BitWidth == Val[I].getBitWidth());

Add the 'initializes' attribute langref and support #84803

Are you sure you want to change the base?

Add the 'initializes' attribute langref and support #84803

Conversation

haopliu commented Mar 11, 2024 • edited

llvmbot commented Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeubanks commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeubanks commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Mar 13, 2024

haopliu commented Mar 14, 2024 • edited

haopliu commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Mar 15, 2024

aeubanks commented Mar 25, 2024 • edited

aeubanks commented Mar 25, 2024

aeubanks commented Mar 25, 2024

aeubanks commented Mar 26, 2024

haopliu commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haopliu commented May 6, 2024 • edited

aeubanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvoung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haopliu commented Mar 11, 2024 •

edited

llvmbot commented Mar 11, 2024 •

edited

haopliu commented Mar 14, 2024 •

edited

aeubanks commented Mar 25, 2024 •

edited

haopliu commented May 6, 2024 •

edited