Allow empty range attribute and add assert for full range #100601

andjo403 · 2024-07-25T17:09:13Z

alternative to #100465

fix #99619

llvmbot · 2024-07-25T17:09:45Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Andreas Jonson (andjo403)

Changes

alternative to #100465

fix #99619

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

4 Files Affected:

(modified) llvm/include/llvm/IR/Attributes.h (+1)
(modified) llvm/lib/Bitcode/Reader/BitcodeReader.cpp (+3-1)
(modified) llvm/lib/IR/Attributes.cpp (+8-1)
(modified) llvm/lib/IR/Core.cpp (+4-4)

diff --git a/llvm/include/llvm/IR/Attributes.h b/llvm/include/llvm/IR/Attributes.h
index 5a80a072dbbd2..cefbe1d760588 100644
--- a/llvm/include/llvm/IR/Attributes.h
+++ b/llvm/include/llvm/IR/Attributes.h
@@ -160,6 +160,7 @@ class Attribute {
   static Attribute getWithUWTableKind(LLVMContext &Context, UWTableKind Kind);
   static Attribute getWithMemoryEffects(LLVMContext &Context, MemoryEffects ME);
   static Attribute getWithNoFPClass(LLVMContext &Context, FPClassTest Mask);
+  static Attribute getWithRange(LLVMContext &Context, const ConstantRange &CR);
 
   /// For a typed attribute, return the equivalent attribute with the type
   /// changed to \p ReplacementTy.
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index fd4ae109b4bb8..091065cf6d55f 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -2369,7 +2369,9 @@ Error BitcodeReader::parseAttributeGroupBlock() {
             return MaybeCR.takeError();
           i--;
 
-          B.addConstantRangeAttr(Kind, MaybeCR.get());
+          assert(Kind == Attribute::Range &&
+                 "Unhandled ConstantRangeAttribute");
+          B.addRangeAttr(MaybeCR.get());
         } else if (Record[i] == 8) {
           Attribute::AttrKind Kind;
 
diff --git a/llvm/lib/IR/Attributes.cpp b/llvm/lib/IR/Attributes.cpp
index b2fdd218e5d4b..4558e8081a67b 100644
--- a/llvm/lib/IR/Attributes.cpp
+++ b/llvm/lib/IR/Attributes.cpp
@@ -286,6 +286,13 @@ Attribute Attribute::getWithNoFPClass(LLVMContext &Context,
   return get(Context, NoFPClass, ClassMask);
 }
 
+Attribute Attribute::getWithRange(LLVMContext &Context,
+                                  const ConstantRange &CR) {
+  assert(!CR.isEmptySet() && "Range must not be empty!");
+  assert(!CR.isFullSet() && "Range must not be full!");
+  return get(Context, Range, CR);
+}
+
 Attribute
 Attribute::getWithAllocSizeArgs(LLVMContext &Context, unsigned ElemSizeArg,
                                 const std::optional<unsigned> &NumElemsArg) {
@@ -2024,7 +2031,7 @@ AttrBuilder &AttrBuilder::addConstantRangeAttr(Attribute::AttrKind Kind,
 }
 
 AttrBuilder &AttrBuilder::addRangeAttr(const ConstantRange &CR) {
-  return addConstantRangeAttr(Attribute::Range, CR);
+  return addAttribute(Attribute::getWithRange(Ctx, CR));
 }
 
 AttrBuilder &
diff --git a/llvm/lib/IR/Core.cpp b/llvm/lib/IR/Core.cpp
index 17c0bf72ef05d..f3e0ed385ae2b 100644
--- a/llvm/lib/IR/Core.cpp
+++ b/llvm/lib/IR/Core.cpp
@@ -191,11 +191,11 @@ LLVMAttributeRef LLVMCreateConstantRangeAttribute(LLVMContextRef C,
                                                   const uint64_t UpperWords[]) {
   auto &Ctx = *unwrap(C);
   auto AttrKind = (Attribute::AttrKind)KindID;
+  assert(AttrKind == Attribute::Range && "Unhandled ConstantRangeAttribute");
   unsigned NumWords = divideCeil(NumBits, 64);
-  return wrap(Attribute::get(
-      Ctx, AttrKind,
-      ConstantRange(APInt(NumBits, ArrayRef(LowerWords, NumWords)),
-                    APInt(NumBits, ArrayRef(UpperWords, NumWords)))));
+  return wrap(Attribute::getWithRange(
+      Ctx, ConstantRange(APInt(NumBits, ArrayRef(LowerWords, NumWords)),
+                         APInt(NumBits, ArrayRef(UpperWords, NumWords)))));
 }
 
 LLVMAttributeRef LLVMCreateStringAttribute(LLVMContextRef C,

nikic · 2024-07-28T10:53:29Z

I'm wondering whether we wouldn't be better off allowing empty ranges. I'm pretty sure that patches like #100899 are going to cause failures in edge cases (where ctpop is in dead code and we infer that no values are valid).

Similarly, if we have an existing range attribute and then want to add new information to it and we perform an intersection, we may end up with an empty range. In that case we'd need a special case to leave the original information alone.

I think things will overall be more robust if we make empty ranges valid (it just means that the value will always be poison).

andjo403 · 2024-07-28T11:30:15Z

Yes I also started to think about in how may places that needed to have that type of empty/full check to avoid asserts when I looked at that PR, I was not able to think of a case but as you say probably missed some corner case.
Do you think that if we remove the empty case in the LangRef that we also shall remove the full case feels like that is only an hint to not add ranges that do not add any information.
so remove this line The range should not represent the full or empty set. That is, a!=b.

nikic · 2024-07-28T14:04:06Z

I think it would be somewhat unusual to allow no-op attributes, e.g. we don't do this for some other things like dereferenceable(0) either. Instead, some key places will simply not add the attribute if it would have no effect (e.g.

llvm-project/llvm/lib/IR/Attributes.cpp

Lines 1932 to 1936 in bb06453

    
           AttrBuilder &AttrBuilder::addDereferenceableAttr(uint64_t Bytes) { 
        
             if (Bytes == 0) return *this; 
        
             return addRawIntAttr(Attribute::Dereferenceable, Bytes); 
        
           }

).

Though my main concern there would be that in textual IR it's kind of hard to distinguish empty & full range without knowing LLVM implementation details, as they are both represented using lo=hi.

andjo403 · 2024-07-28T14:13:09Z

hmm so the ref shall be something like The range should not represent the full set. That is, a!=b, Exception is the empty set where a and b is the min value.

andjo403 · 2024-07-28T14:32:12Z

or keep the lang ref as is and do as AttrBuilder::addDereferenceableAttr that we only add the attribute if not empty/full

nikic · 2024-07-29T14:15:39Z

or keep the lang ref as is and do as AttrBuilder::addDereferenceableAttr that we only add the attribute if not empty/full

Not sure about just silently dropping the empty case. For full, this is equivalent to not having the attribute, so that makes sense. For empty, this will result in some non-monotonic behavior. E.g. if you originally have range(0, 5) and then intersect with range(4, 10) you get range(4, 5), but if you intersect with range(5, 10) you instead keep range(0, 5). That is, inferring a better range results in worse attributes being materialized. That doesn't break anything, but it's probably still undesirable.

andjo403 · 2024-08-06T19:42:42Z

Updated the langref to allow empty ranges and asserts for full range but the builder ignores full ranges

nikic · 2024-08-07T12:57:48Z

llvm/docs/LangRef.rst

+    -  Only for the empty set is ``a`` and ``b`` allowed to be equal and
+       only for the value 0.


I found this a bit hard to understand, maybe like this?

Suggested change

- Only for the empty set is ``a`` and ``b`` allowed to be equal and

only for the value 0.

- The empty range is represented using ``0,0``.

- Otherwise, ``a`` and ``b` are not allowed to be equal.

yes much better

nikic · 2024-08-07T12:58:11Z

llvm/lib/AsmParser/LLParser.cpp

@@ -3109,8 +3109,8 @@ bool LLParser::parseRangeAttr(AttrBuilder &B) {
  if (ParseAPSInt(BitWidth, Lower) ||
      parseToken(lltok::comma, "expected ','") || ParseAPSInt(BitWidth, Upper))
    return true;
-  if (Lower == Upper)
-    return tokError("the range should not represent the full or empty set!");
+  if (Lower == Upper && !Lower.isMinValue())


Suggested change

if (Lower == Upper && !Lower.isMinValue())

if (Lower == Upper && !Lower.isZero())

nikic · 2024-08-07T12:59:19Z

llvm/lib/IR/Attributes.cpp

@@ -286,6 +286,12 @@ Attribute Attribute::getWithNoFPClass(LLVMContext &Context,
  return get(Context, NoFPClass, ClassMask);
 }

+Attribute Attribute::getWithRange(LLVMContext &Context,
+                                  const ConstantRange &CR) {
+  assert(!CR.isFullSet() && "Range must not be full!");


I think it would be okay to just add this directly into the generic ctor, rather than specific to range. I would expect that other range-based attributes added in the future will not allow full ranges either. (If they do, we can separate it out at the time.)

yes that seems resonable.

nikic

LGTM

andjo403 requested a review from nikic as a code owner July 25, 2024 17:09

llvmbot added the llvm:ir label Jul 25, 2024

andjo403 force-pushed the rangeAssert branch from 4c89f96 to 5755362 Compare July 28, 2024 15:53

llvmbot added the llvm:transforms label Jul 28, 2024

andjo403 force-pushed the rangeAssert branch from 5755362 to ffd7c16 Compare August 6, 2024 19:39

andjo403 changed the title ~~Assert range attribute is not empty nor full~~ Allow empty range attribute and add assert for full range Aug 6, 2024

nikic reviewed Aug 7, 2024

View reviewed changes

Add pre-commit tests. NFC

7dc976d

andjo403 force-pushed the rangeAssert branch from ffd7c16 to 6ea381c Compare August 7, 2024 17:01

Allow empty range attribute and add assert for full range

5565f4c

andjo403 force-pushed the rangeAssert branch from 6ea381c to 5565f4c Compare August 7, 2024 18:04

nikic approved these changes Aug 8, 2024

View reviewed changes

andjo403 merged commit 04da773 into llvm:main Aug 8, 2024
8 checks passed

andjo403 deleted the rangeAssert branch August 8, 2024 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow empty range attribute and add assert for full range #100601

Allow empty range attribute and add assert for full range #100601

Uh oh!

andjo403 commented Jul 25, 2024

Uh oh!

llvmbot commented Jul 25, 2024 •

edited

Loading

Uh oh!

nikic commented Jul 28, 2024

Uh oh!

andjo403 commented Jul 28, 2024

Uh oh!

nikic commented Jul 28, 2024

Uh oh!

andjo403 commented Jul 28, 2024

Uh oh!

andjo403 commented Jul 28, 2024

Uh oh!

nikic commented Jul 29, 2024

Uh oh!

andjo403 commented Aug 6, 2024

Uh oh!

nikic Aug 7, 2024

Uh oh!

andjo403 Aug 7, 2024

Uh oh!

nikic Aug 7, 2024

Uh oh!

nikic Aug 7, 2024

Uh oh!

andjo403 Aug 7, 2024

Uh oh!

nikic left a comment

Uh oh!

Uh oh!

Uh oh!

		- Only for the empty set is ``a`` and ``b`` allowed to be equal and
		only for the value 0.

	if (Lower == Upper && !Lower.isMinValue())
	if (Lower == Upper && !Lower.isZero())

Allow empty range attribute and add assert for full range #100601

Allow empty range attribute and add assert for full range #100601

Uh oh!

Conversation

andjo403 commented Jul 25, 2024

Uh oh!

llvmbot commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Jul 28, 2024

Uh oh!

andjo403 commented Jul 28, 2024

Uh oh!

nikic commented Jul 28, 2024

Uh oh!

andjo403 commented Jul 28, 2024

Uh oh!

andjo403 commented Jul 28, 2024

Uh oh!

nikic commented Jul 29, 2024

Uh oh!

andjo403 commented Aug 6, 2024

Uh oh!

nikic Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

andjo403 Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

nikic Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

nikic Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

andjo403 Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jul 25, 2024 •

edited

Loading