[IR][ModRef] Introduce `errno` memory location #120783

antoniofrighetto · 2024-12-20T18:40:04Z

Model C/C++ errno macro by adding a corresponding errno memory location kind to the IR. Preliminary work to separate errno writes from other memory accesses, to the benefit of alias analyses and optimization correctness.

Previous discussion: https://discourse.llvm.org/t/rfc-modelling-errno-memory-effects/82972.

llvmbot · 2024-12-20T18:40:47Z

@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-mlir-llvm
@llvm/pr-subscribers-mlir
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-support

Author: Antonio Frighetto (antoniofrighetto)

Changes

Model C/C++ errno macro by adding a corresponding errno memory location kind to the IR. Preliminary work to separate errno writes from other memory accesses, to the benefit of alias analyses and optimization correctness.

Previous discussion: https://discourse.llvm.org/t/rfc-modelling-errno-memory-effects/82972.

Patch is 33.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/120783.diff

21 Files Affected:

(modified) llvm/include/llvm/AsmParser/LLToken.h (+2)
(modified) llvm/include/llvm/IR/Function.h (+4)
(modified) llvm/include/llvm/IR/InstrTypes.h (+4)
(modified) llvm/include/llvm/Support/ModRef.h (+18-1)
(modified) llvm/lib/AsmParser/LLLexer.cpp (+1)
(modified) llvm/lib/AsmParser/LLParser.cpp (+6-1)
(modified) llvm/lib/IR/Attributes.cpp (+3)
(modified) llvm/lib/IR/Function.cpp (+8)
(modified) llvm/lib/IR/Instructions.cpp (+8)
(modified) llvm/lib/Support/ModRef.cpp (+3)
(modified) llvm/test/Assembler/memory-attribute-errors.ll (+3-3)
(modified) llvm/test/Assembler/memory-attribute.ll (+12)
(modified) llvm/test/Transforms/FunctionAttrs/argmemonly.ll (+11-11)
(modified) llvm/test/Transforms/FunctionAttrs/nocapture.ll (+15-15)
(modified) llvm/test/Transforms/FunctionAttrs/read-write-scc.ll (+2-2)
(modified) llvm/test/Transforms/FunctionAttrs/readattrs.ll (+1-1)
(modified) llvm/test/Transforms/FunctionAttrs/writeonly.ll (+2-2)
(modified) llvm/test/Transforms/InferFunctionAttrs/norecurse_debug.ll (+1-1)
(modified) llvm/test/Transforms/LowerTypeTests/cfi-nounwind-direct-call.ll (+1-1)
(modified) llvm/test/Transforms/SCCP/ipscp-drop-argmemonly.ll (+6-6)
(modified) llvm/unittests/Support/ModRefTest.cpp (+2-2)

diff --git a/llvm/include/llvm/AsmParser/LLToken.h b/llvm/include/llvm/AsmParser/LLToken.h
index 178c911120b4ce..ac0887f8e5fb52 100644
--- a/llvm/include/llvm/AsmParser/LLToken.h
+++ b/llvm/include/llvm/AsmParser/LLToken.h
@@ -201,11 +201,13 @@ enum Kind {
   kw_readwrite,
   kw_argmem,
   kw_inaccessiblemem,
+  kw_errnomem,
 
   // Legacy memory attributes:
   kw_argmemonly,
   kw_inaccessiblememonly,
   kw_inaccessiblemem_or_argmemonly,
+  kw_errnomemonly,
 
   // nofpclass attribute:
   kw_all,
diff --git a/llvm/include/llvm/IR/Function.h b/llvm/include/llvm/IR/Function.h
index e7afcbd31420c1..bb8e6d5a7e38ec 100644
--- a/llvm/include/llvm/IR/Function.h
+++ b/llvm/include/llvm/IR/Function.h
@@ -575,6 +575,10 @@ class LLVM_ABI Function : public GlobalObject, public ilist_node<Function> {
   bool onlyAccessesInaccessibleMemory() const;
   void setOnlyAccessesInaccessibleMemory();
 
+  /// Determine if the function may only access errno memory.
+  bool onlyAccessesErrnoMemory() const;
+  void setOnlyAccessesErrnoMemory();
+
   /// Determine if the function may only access memory that is
   ///  either inaccessible from the IR or pointed to by its arguments.
   bool onlyAccessesInaccessibleMemOrArgMem() const;
diff --git a/llvm/include/llvm/IR/InstrTypes.h b/llvm/include/llvm/IR/InstrTypes.h
index e6332a16df7d5f..ba9a5c21467ec4 100644
--- a/llvm/include/llvm/IR/InstrTypes.h
+++ b/llvm/include/llvm/IR/InstrTypes.h
@@ -1907,6 +1907,10 @@ class CallBase : public Instruction {
   bool onlyAccessesInaccessibleMemory() const;
   void setOnlyAccessesInaccessibleMemory();
 
+  /// Determine if the function may only access errno memory.
+  bool onlyAccessesErrnoMemory() const;
+  void setOnlyAccessesErrnoMemory();
+
   /// Determine if the function may only access memory that is
   /// either inaccessible from the IR or pointed to by its arguments.
   bool onlyAccessesInaccessibleMemOrArgMem() const;
diff --git a/llvm/include/llvm/Support/ModRef.h b/llvm/include/llvm/Support/ModRef.h
index 5a9d80c87ae27a..b5071246d36c2f 100644
--- a/llvm/include/llvm/Support/ModRef.h
+++ b/llvm/include/llvm/Support/ModRef.h
@@ -61,8 +61,10 @@ enum class IRMemLocation {
   ArgMem = 0,
   /// Memory that is inaccessible via LLVM IR.
   InaccessibleMem = 1,
+  /// Errno memory.
+  ErrnoMem = 2,
   /// Any other memory.
-  Other = 2,
+  Other = 3,
 
   /// Helpers to iterate all locations in the MemoryEffectsBase class.
   First = ArgMem,
@@ -139,6 +141,11 @@ template <typename LocationEnum> class MemoryEffectsBase {
     return MemoryEffectsBase(Location::InaccessibleMem, MR);
   }
 
+  /// Create MemoryEffectsBase that can only access errno memory.
+  static MemoryEffectsBase errnoMemOnly(ModRefInfo MR = ModRefInfo::ModRef) {
+    return MemoryEffectsBase(Location::ErrnoMem, MR);
+  }
+
   /// Create MemoryEffectsBase that can only access inaccessible or argument
   /// memory.
   static MemoryEffectsBase
@@ -207,11 +214,21 @@ template <typename LocationEnum> class MemoryEffectsBase {
     return isModOrRefSet(getModRef(Location::ArgMem));
   }
 
+  /// Whether this function may access errno memory.
+  bool doesAccessErrnoMem() const {
+    return isModOrRefSet(getModRef(Location::ErrnoMem));
+  }
+
   /// Whether this function only (at most) accesses inaccessible memory.
   bool onlyAccessesInaccessibleMem() const {
     return getWithoutLoc(Location::InaccessibleMem).doesNotAccessMemory();
   }
 
+  /// Whether this function only (at most) accesses errno memory.
+  bool onlyAccessesErrnoMem() const {
+    return getWithoutLoc(Location::ErrnoMem).doesNotAccessMemory();
+  }
+
   /// Whether this function only (at most) accesses argument and inaccessible
   /// memory.
   bool onlyAccessesInaccessibleOrArgMem() const {
diff --git a/llvm/lib/AsmParser/LLLexer.cpp b/llvm/lib/AsmParser/LLLexer.cpp
index 1b8e033134f51b..e9d1f7c93e1089 100644
--- a/llvm/lib/AsmParser/LLLexer.cpp
+++ b/llvm/lib/AsmParser/LLLexer.cpp
@@ -701,6 +701,7 @@ lltok::Kind LLLexer::LexIdentifier() {
   KEYWORD(readwrite);
   KEYWORD(argmem);
   KEYWORD(inaccessiblemem);
+  KEYWORD(errnomem);
   KEYWORD(argmemonly);
   KEYWORD(inaccessiblememonly);
   KEYWORD(inaccessiblemem_or_argmemonly);
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index 52d48a69f0eb53..ffd9a7953bec8d 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -1668,6 +1668,9 @@ static bool upgradeMemoryAttr(MemoryEffects &ME, lltok::Kind Kind) {
   case lltok::kw_inaccessiblememonly:
     ME &= MemoryEffects::inaccessibleMemOnly();
     return true;
+  case lltok::kw_errnomemonly:
+    ME &= MemoryEffects::errnoMemOnly();
+    return true;
   case lltok::kw_inaccessiblemem_or_argmemonly:
     ME &= MemoryEffects::inaccessibleOrArgMemOnly();
     return true;
@@ -2488,6 +2491,8 @@ static std::optional<MemoryEffects::Location> keywordToLoc(lltok::Kind Tok) {
     return IRMemLocation::ArgMem;
   case lltok::kw_inaccessiblemem:
     return IRMemLocation::InaccessibleMem;
+  case lltok::kw_errnomem:
+    return IRMemLocation::ErrnoMem;
   default:
     return std::nullopt;
   }
@@ -2536,7 +2541,7 @@ std::optional<MemoryEffects> LLParser::parseMemoryAttr() {
     std::optional<ModRefInfo> MR = keywordToModRef(Lex.getKind());
     if (!MR) {
       if (!Loc)
-        tokError("expected memory location (argmem, inaccessiblemem) "
+        tokError("expected memory location (argmem, inaccessiblemem, errnomem) "
                  "or access kind (none, read, write, readwrite)");
       else
         tokError("expected access kind (none, read, write, readwrite)");
diff --git a/llvm/lib/IR/Attributes.cpp b/llvm/lib/IR/Attributes.cpp
index e9daa01b899e8f..94bd16df120e30 100644
--- a/llvm/lib/IR/Attributes.cpp
+++ b/llvm/lib/IR/Attributes.cpp
@@ -637,6 +637,9 @@ std::string Attribute::getAsString(bool InAttrGrp) const {
       case IRMemLocation::InaccessibleMem:
         OS << "inaccessiblemem: ";
         break;
+      case IRMemLocation::ErrnoMem:
+        OS << "errnomem: ";
+        break;
       case IRMemLocation::Other:
         llvm_unreachable("This is represented as the default access kind");
       }
diff --git a/llvm/lib/IR/Function.cpp b/llvm/lib/IR/Function.cpp
index 9c5dd5aeb92e97..616b2c98d57b07 100644
--- a/llvm/lib/IR/Function.cpp
+++ b/llvm/lib/IR/Function.cpp
@@ -922,6 +922,14 @@ void Function::setOnlyAccessesInaccessibleMemory() {
   setMemoryEffects(getMemoryEffects() & MemoryEffects::inaccessibleMemOnly());
 }
 
+/// Determine if the function may only access errno memory.
+bool Function::onlyAccessesErrnoMemory() const {
+  return getMemoryEffects().onlyAccessesErrnoMem();
+}
+void Function::setOnlyAccessesErrnoMemory() {
+  setMemoryEffects(getMemoryEffects() & MemoryEffects::errnoMemOnly());
+}
+
 /// Determine if the function may only access memory that is
 ///  either inaccessible from the IR or pointed to by its arguments.
 bool Function::onlyAccessesInaccessibleMemOrArgMem() const {
diff --git a/llvm/lib/IR/Instructions.cpp b/llvm/lib/IR/Instructions.cpp
index 2d6fe40f4c1de0..e9c01282985308 100644
--- a/llvm/lib/IR/Instructions.cpp
+++ b/llvm/lib/IR/Instructions.cpp
@@ -665,6 +665,14 @@ void CallBase::setOnlyAccessesInaccessibleMemory() {
   setMemoryEffects(getMemoryEffects() & MemoryEffects::inaccessibleMemOnly());
 }
 
+/// Determine if the function may only access errno memory.
+bool CallBase::onlyAccessesErrnoMemory() const {
+  return getMemoryEffects().onlyAccessesErrnoMem();
+}
+void CallBase::setOnlyAccessesErrnoMemory() {
+  setMemoryEffects(getMemoryEffects() & MemoryEffects::errnoMemOnly());
+}
+
 /// Determine if the function may only access memory that is
 ///  either inaccessible from the IR or pointed to by its arguments.
 bool CallBase::onlyAccessesInaccessibleMemOrArgMem() const {
diff --git a/llvm/lib/Support/ModRef.cpp b/llvm/lib/Support/ModRef.cpp
index a4eb70edd38d10..44057095369e0e 100644
--- a/llvm/lib/Support/ModRef.cpp
+++ b/llvm/lib/Support/ModRef.cpp
@@ -42,6 +42,9 @@ raw_ostream &llvm::operator<<(raw_ostream &OS, MemoryEffects ME) {
     case IRMemLocation::InaccessibleMem:
       OS << "InaccessibleMem: ";
       break;
+    case IRMemLocation::ErrnoMem:
+      OS << "ErrnoMem: ";
+      break;
     case IRMemLocation::Other:
       OS << "Other: ";
       break;
diff --git a/llvm/test/Assembler/memory-attribute-errors.ll b/llvm/test/Assembler/memory-attribute-errors.ll
index 1fba90362e79b9..2eed11d9465d58 100644
--- a/llvm/test/Assembler/memory-attribute-errors.ll
+++ b/llvm/test/Assembler/memory-attribute-errors.ll
@@ -12,16 +12,16 @@
 ; MISSING-ARGS: error: expected '('
 declare void @fn() memory
 ;--- empty.ll
-; EMPTY: error: expected memory location (argmem, inaccessiblemem) or access kind (none, read, write, readwrite)
+; EMPTY: error: expected memory location (argmem, inaccessiblemem, errnomem) or access kind (none, read, write, readwrite)
 declare void @fn() memory()
 ;--- unterminated.ll
 ; UNTERMINATED: error: unterminated memory attribute
 declare void @fn() memory(read
 ;--- invalid-kind.ll
-; INVALID-KIND: error: expected memory location (argmem, inaccessiblemem) or access kind (none, read, write, readwrite)
+; INVALID-KIND: error: expected memory location (argmem, inaccessiblemem, errnomem) or access kind (none, read, write, readwrite)
 declare void @fn() memory(foo)
 ;--- other.ll
-; OTHER: error: expected memory location (argmem, inaccessiblemem) or access kind (none, read, write, readwrite)
+; OTHER: error: expected memory location (argmem, inaccessiblemem, errnomem) or access kind (none, read, write, readwrite)
 declare void @fn() memory(other: read)
 ;--- missing-colon.ll
 ; MISSING-COLON: error: expected ':' after location
diff --git a/llvm/test/Assembler/memory-attribute.ll b/llvm/test/Assembler/memory-attribute.ll
index 2f7d3980eb378b..effd4ce7c45483 100644
--- a/llvm/test/Assembler/memory-attribute.ll
+++ b/llvm/test/Assembler/memory-attribute.ll
@@ -40,6 +40,18 @@ declare void @fn_inaccessiblemem_write() memory(inaccessiblemem: write)
 ; CHECK: @fn_inaccessiblemem_readwrite()
 declare void @fn_inaccessiblemem_readwrite() memory(inaccessiblemem: readwrite)
 
+; CHECK: Function Attrs: memory(errnomem: read)
+; CHECK: @fn_errnomem_read()
+declare void @fn_errnomem_read() memory(errnomem: read)
+
+; CHECK: Function Attrs: memory(errnomem: write)
+; CHECK: @fn_errnomem_write()
+declare void @fn_errnomem_write() memory(errnomem: write)
+
+; CHECK: Function Attrs: memory(errnomem: readwrite)
+; CHECK: @fn_errnomem_readwrite()
+declare void @fn_errnomem_readwrite() memory(errnomem: readwrite)
+
 ; CHECK: Function Attrs: memory(read, argmem: readwrite)
 ; CHECK: @fn_read_argmem_readwrite()
 declare void @fn_read_argmem_readwrite() memory(read, argmem: readwrite)
diff --git a/llvm/test/Transforms/FunctionAttrs/argmemonly.ll b/llvm/test/Transforms/FunctionAttrs/argmemonly.ll
index 5bbe6fa7c27c2e..6f3bd8c5ae0de6 100644
--- a/llvm/test/Transforms/FunctionAttrs/argmemonly.ll
+++ b/llvm/test/Transforms/FunctionAttrs/argmemonly.ll
@@ -56,7 +56,7 @@ entry:
 }
 
 define i32 @test_read_global() {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, argmem: none, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, argmem: none, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define i32 @test_read_global
 ; FNATTRS-SAME: () #[[ATTR2:[0-9]+]] {
 ; FNATTRS-NEXT:  entry:
@@ -76,7 +76,7 @@ entry:
 }
 
 define i32 @test_read_loaded_ptr(ptr %ptr) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define i32 @test_read_loaded_ptr
 ; FNATTRS-SAME: (ptr nocapture readonly [[PTR:%.*]]) #[[ATTR3:[0-9]+]] {
 ; FNATTRS-NEXT:  entry:
@@ -119,7 +119,7 @@ entry:
 }
 
 define void @test_write_global() {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(write, argmem: none, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(write, argmem: none, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define void @test_write_global
 ; FNATTRS-SAME: () #[[ATTR5:[0-9]+]] {
 ; FNATTRS-NEXT:  entry:
@@ -243,7 +243,7 @@ declare void @llvm.memcpy.p0.p0.i64(ptr, ptr, i64, i1)
 @arr = global [32 x i8] zeroinitializer
 
 define void @test_memcpy_src_global(ptr %dst) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define void @test_memcpy_src_global
 ; FNATTRS-SAME: (ptr nocapture writeonly initializes((0, 32)) [[DST:%.*]]) #[[ATTR11:[0-9]+]] {
 ; FNATTRS-NEXT:  entry:
@@ -263,7 +263,7 @@ entry:
 }
 
 define void @test_memcpy_dst_global(ptr %src) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define void @test_memcpy_dst_global
 ; FNATTRS-SAME: (ptr nocapture readonly [[SRC:%.*]]) #[[ATTR11]] {
 ; FNATTRS-NEXT:  entry:
@@ -388,7 +388,7 @@ define void @test_inaccessibleorargmemonly_readwrite(ptr %arg) {
 }
 
 define void @test_recursive_argmem_read(ptr %p) {
-; FNATTRS: Function Attrs: nofree nosync nounwind memory(read, inaccessiblemem: none)
+; FNATTRS: Function Attrs: nofree nosync nounwind memory(read, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define void @test_recursive_argmem_read
 ; FNATTRS-SAME: (ptr nocapture readonly [[P:%.*]]) #[[ATTR16:[0-9]+]] {
 ; FNATTRS-NEXT:    [[PVAL:%.*]] = load ptr, ptr [[P]], align 8
@@ -408,7 +408,7 @@ define void @test_recursive_argmem_read(ptr %p) {
 }
 
 define void @test_recursive_argmem_readwrite(ptr %p) {
-; FNATTRS: Function Attrs: nofree nosync nounwind memory(readwrite, inaccessiblemem: none)
+; FNATTRS: Function Attrs: nofree nosync nounwind memory(readwrite, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define void @test_recursive_argmem_readwrite
 ; FNATTRS-SAME: (ptr nocapture [[P:%.*]]) #[[ATTR17:[0-9]+]] {
 ; FNATTRS-NEXT:    [[PVAL:%.*]] = load ptr, ptr [[P]], align 8
@@ -454,7 +454,7 @@ define void @test_recursive_argmem_read_alloca(ptr %p) {
 }
 
 define void @test_scc_argmem_read_1(ptr %p) {
-; FNATTRS: Function Attrs: nofree nosync nounwind memory(read, inaccessiblemem: none)
+; FNATTRS: Function Attrs: nofree nosync nounwind memory(read, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define void @test_scc_argmem_read_1
 ; FNATTRS-SAME: (ptr nocapture readonly [[P:%.*]]) #[[ATTR16]] {
 ; FNATTRS-NEXT:    [[PVAL:%.*]] = load ptr, ptr [[P]], align 8
@@ -474,7 +474,7 @@ define void @test_scc_argmem_read_1(ptr %p) {
 }
 
 define void @test_scc_argmem_read_2(ptr %p) {
-; FNATTRS: Function Attrs: nofree nosync nounwind memory(read, inaccessiblemem: none)
+; FNATTRS: Function Attrs: nofree nosync nounwind memory(read, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define void @test_scc_argmem_read_2
 ; FNATTRS-SAME: (ptr nocapture readonly [[P:%.*]]) #[[ATTR16]] {
 ; FNATTRS-NEXT:    call void @test_scc_argmem_read_1(ptr [[P]])
@@ -518,7 +518,7 @@ entry:
 
 ; FIXME: This could be `memory(argmem: read)`.
 define i64 @select_different_obj(i1 %c, ptr %p, ptr %p2) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define i64 @select_different_obj
 ; FNATTRS-SAME: (i1 [[C:%.*]], ptr nocapture readonly [[P:%.*]], ptr nocapture readonly [[P2:%.*]]) #[[ATTR3]] {
 ; FNATTRS-NEXT:  entry:
@@ -580,7 +580,7 @@ join:
 
 ; FIXME: This could be `memory(argmem: read)`.
 define i64 @phi_different_obj(i1 %c, ptr %p, ptr %p2) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define i64 @phi_different_obj
 ; FNATTRS-SAME: (i1 [[C:%.*]], ptr nocapture readonly [[P:%.*]], ptr nocapture readonly [[P2:%.*]]) #[[ATTR3]] {
 ; FNATTRS-NEXT:  entry:
diff --git a/llvm/test/Transforms/FunctionAttrs/nocapture.ll b/llvm/test/Transforms/FunctionAttrs/nocapture.ll
index 7df6132ac6a315..ce2373ce574676 100644
--- a/llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ b/llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -20,7 +20,7 @@ define ptr @c1(ptr %q) {
 
 ; It would also be acceptable to mark %q as readnone. Update @c3 too.
 define void @c2(ptr %q) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(write, argmem: none, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(write, argmem: none, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define void @c2
 ; FNATTRS-SAME: (ptr [[Q:%.*]]) #[[ATTR1:[0-9]+]] {
 ; FNATTRS-NEXT:    store ptr [[Q]], ptr @g, align 8
@@ -37,7 +37,7 @@ define void @c2(ptr %q) {
 }
 
 define void @c3(ptr %q) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(write, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(write, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define void @c3
 ; FNATTRS-SAME: (ptr [[Q:%.*]]) #[[ATTR2:[0-9]+]] {
 ; FNATTRS-NEXT:    call void @c2(ptr [[Q]])
@@ -127,7 +127,7 @@ l1:
 @lookup_table = global [2 x i1] [ i1 0, i1 1 ]
 
 define i1 @c5(ptr %q, i32 %bitno) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, argmem: none, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, argmem: none, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define i1 @c5
 ; FNATTRS-SAME: (ptr [[Q:%.*]], i32 [[BITNO:%.*]]) #[[ATTR3:[0-9]+]] {
 ; FNATTRS-NEXT:    [[TMP:%.*]] = ptrtoint ptr [[Q]] to i32
@@ -222,7 +222,7 @@ define ptr @lookup_bit(ptr %q, i32 %bitno) readnone nounwind {
 }
 
 define i1 @c7(ptr %q, i32 %bitno) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(read, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define i1 @c7
 ; FNATTRS-SAME: (ptr readonly [[Q:%.*]], i32 [[BITNO:%.*]]) #[[ATTR6:[0-9]+]] {
 ; FNATTRS-NEXT:    [[PTR:%.*]] = call ptr @lookup_bit(ptr [[Q]], i32 [[BITNO]])
@@ -243,7 +243,7 @@ define i1 @c7(ptr %q, i32 %bitno) {
 
 
 define i32 @nc1(ptr %q, ptr %p, i1 %b) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define i32 @nc1
 ; FNATTRS-SAME: (ptr [[Q:%.*]], ptr nocapture [[P:%.*]], i1 [[B:%.*]]) #[[ATTR7:[0-9]+]] {
 ; FNATTRS-NEXT:  e:
@@ -284,7 +284,7 @@ l:
 }
 
 define i32 @nc1_addrspace(ptr %q, ptr addrspace(1) %p, i1 %b) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, inaccessiblemem: none, errnomem: none)
 ; FNATTRS-LABEL: define i32 @nc1_addrspace
 ; FNATTRS-SAME: (ptr [[Q:%.*]], ptr addrspace(1) nocapture [[P:%.*]], i1 [[B:%.*]]) #[[ATTR7]] {
 ; FNATTRS-NEXT:  e:
@@ -328,7 +328,7 @@ l:
 }
 
 define void @nc2(ptr %p, ptr %q) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, inaccessiblemem: none)
+; FNATTRS: F...
[truncated]

antoniofrighetto · 2025-01-09T08:56:11Z

Kind ping for correct direction.

efriedma-quic

This seems like roughly what I expected from the proposal.

efriedma-quic · 2025-01-10T23:39:12Z

llvm/test/Transforms/FunctionAttrs/writeonly.ll

@@ -82,7 +82,7 @@ define void @test_store(ptr %p) {

 @G = external global ptr
 define i8 @test_store_capture(ptr %p) {
-; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, argmem: read, inaccessiblemem: none)
+; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, argmem: read, inaccessiblemem: none, errnomem: none)


How are we inferring errnomem: none here?

While walking the call-graph for deducing attrs, in checkFunctionMemoryAccess we initialize a ME object that doesn't access/modify memory and gradually refine it (currently says ErrnoMem: NoModRef). This ME is intersected with the original ME based on AA results (which says ErrnoMem: ModRef), so we get NoModRef, as per the meet of the lattice. We miss logic to infer if the function clobbers errno, but I think it should be fine as the PR only introduces the location (it might be better to conservatively always say ModRef, but the location is currently unused, so it should be okay to address this in an upcoming PR?). Rebased to main too.

I'd prefer to get this right from the start, to avoid any future confusion.

Should be now taken into account while checking if a memory location is related to errno in addLocAccess by looking at the underlying object. Rebased, and added accessesArgMemOrErrnoMem helper too.

This is still wrong in the latest patch?

Hmm, the pointer is never accessed with an integer size, it should never alias errno, so should be correct to infer NoModRef for test_store_capture? Though now I think we might still miss something as follows while inferring attributes (and then let AA conclude if it aliases errno or not)?

if (Loc->Size == LocationSize::precise(sizeof(int))) ME |= MemoryEffects::errnoMemOnly(MR);

The argument %p to test_store_capture can point to errno.

You can't assume all accesses to errno are exactly 32 bits wide. (They usually will be, but if, for example, someone writes (char)errno, we'll narrow the load.) It's probably okay to assume accesses wider than 32 bits don't access errno...

Mind checking that the last fixup commit is more aligned with what you thought?

Hmm, need to take a better look at this.

github-actions · 2025-02-04T17:34:01Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvm/include/llvm/Analysis/MemoryLocation.h

llvm/include/llvm/AsmParser/LLToken.h

llvm/include/llvm/Bitcode/LLVMBitCodes.h

nikic · 2025-02-06T16:11:27Z

llvm/include/llvm/IR/Function.h

+  ///  either inaccessible from the IR, pointed to by its arguments, or errno
+  ///  memory.
+  bool onlyAccessesInaccessibleMemOrArgMemOrErrnoMem() const;
+  void setOnlyAccessesInaccessibleMemOrArgMemOrErrnoMem();


Do not add these combinatorial APIs, unless they are widely useful.

I think there are a handful of libc routines which would benefit from these APIs.

I think even if they are useful for them, there is no need to have them on CallBase. This code should all be centralized in BuildLibCalls, right?

Basically, delay adding any APIs until the patch that introduces at least one use of them :)

That makes sense!

nikic · 2025-02-06T16:11:33Z

llvm/include/llvm/IR/InstrTypes.h

+  ///  either inaccessible from the IR, pointed to by its arguments, or errno
+  ///  memory.
+  bool onlyAccessesInaccessibleMemOrArgMemOrErrnoMem() const;
+  void setOnlyAccessesInaccessibleMemOrArgMemOrErrnoMem();


nikic · 2025-02-06T16:13:06Z

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

+    return true;
+  case bitc::ATTR_KIND_INACCESSIBLEMEM_OR_ARGMEM_OR_ERRNOMEMONLY:
+    ME &= MemoryEffects::inaccessibleOrArgOrErrnoMemOnly();
+    return true;


As said, you don't need all of this upgrade code for attributes that never existed ... but you do need code to upgrade old MemoryEffects, where you need to copy Other to Errno.

A way to do that would be to add a version number to the high byte of the MemoryEffects bitcode encoding.

Thank you for clarifying this, I think it's a bit clearer now. I still have a doubt though: in decodeLLVMAttributesForBitcode, I believe I should expect to have something as follows:

uint64_t Attrs = ((EncodedAttrs & (0xfffffULL << 32)) >> 11) | (EncodedAttrs & 0xffff); uint8_t Version = (EncodedAttrs >> 56) & 0xFF; if (AttrIdx == AttributeList::FunctionIndex) { // Upgrade old memory attributes. // ... // If version < 1 and attribute for Other is set, copy Other ModRef to Errno. if (Version < 1 && (Attrs & (7ULL << 39)) ME &= MemoryEffects::errnoMemOnly(ME.getModRef(IRMemLocation::Other)); if (ME != MemoryEffects::unknown()) B.addMemoryAttr(ME); }

However, we currently miss a mask for attribute Other to check if it was set, so this should be introduced too?

A way to do that would be to add a version number to the high byte of the MemoryEffects bitcode encoding.

As per doc-comment in decodeLLVMAttributesForBitcode, I was expecting to see encodeLLVMAttributesForBitcode in the Writer, where to add a version number, but seems to have been dropped a while ago. Am I understanding correctly that in writeAttributeTable we should somehow still have something as follows?

const uint8_t Version = 1; uint64_t EncodedAttrs = (Version << 56) | ME.getAsInt(); Record.emplace_back(EncodedAttrs);

This looks a little bit confusing though, as we don't seem to be directly manipulating MemoryEffects encoding anymore (as it used to be the case).

decodeLLVMAttributesForBitcode is not relevant for this kind of attribute. The place to perform the upgrade would be here:

llvm-project/llvm/lib/Bitcode/Reader/BitcodeReader.cpp

Lines 2401 to 2402 in 83af335

else if (Kind == Attribute::Memory)

B.addMemoryAttr(MemoryEffects::createFromIntValue(Record[++i]));

And the version can be added on the encoding side here:

llvm-project/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

Line 981 in 83af335

Record.push_back(Attr.getValueAsInt());

This would be handling just for MemoryEffects, not generically for all attributes.

Thank you, I think it should look like towards being more on track now (minor opportunity to drop old comment as well).

nikic · 2025-02-06T16:14:15Z

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

+      ME |= MemoryEffects::errnoMemOnly(MR);
+      return;
+    }
+  }


Let's please not introduce any special inference logic for errno in this patch, we can do this in a followup.

Note that the bit you actually want to modify the the code a few lines below that uses IRMemLocation::Other. That one should not include Errno as well.

That one should not include Errno as well

You meant that we should include the memory effects of Errno, as Other doesn't affect Errno, correct?

nikic · 2025-02-06T16:15:33Z

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

@@ -126,8 +126,21 @@ static void addLocAccess(MemoryEffects &ME, const MemoryLocation &Loc,
    return;
  if (isa<Argument>(UO)) {
    ME |= MemoryEffects::argMemOnly(MR);
+    if (Loc.Size <= LocationSize::precise(sizeof(int)))
+      ME |= MemoryEffects::errnoMemOnly(MR);


This doesn't look right. Even if the argument might alias errno, it's still argmem from the perspective of this function. The caller has to deal with what the argument actually points to.

Dropped the check for now. I'm slightly unsure if it'd still make sense to keep it while examining formal parameters within the call-site (hence, from caller perspective)? Thinking more on checking the memory location size, say, e.g., a memory location possibly refers to a pointer to pointer: this case would be incorrectly excluded from the above check, correct?

nikic

This needs a LangRef update.

llvm/docs/LangRef.rst

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

nikic · 2025-02-11T11:22:53Z

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

+                                                        0x00FFFFFFFFFFFFFFULL);
+            if (Version < 1)
+              ME |= MemoryEffects::errnoMemOnly(
+                  ME.getModRef(IRMemLocation::Other));


This is not going to do the right thing, because the old Other will be located at the new ErrnoMem, so what you'd actually have to do is copy from errno to other. It might be better to full reconstruct the MemoryEffects without going through createFromIntValue for old versions.

In any case, this needs a test. You need to create a bitcode file with the old llvm-as that has something like memory(write, argmem: read) and then check that llvm-dis still produces the same result (instead of memory(errnomem: write, argmem: read) which I think is what happens without upgrade).

Updated, thanks, should have introduced a test earlier. Thus, IIUC, for older versions, errno should be part of default memory.

nikic · 2025-02-11T11:36:46Z

llvm/test/Transforms/SCCP/ipscp-drop-argmemonly.ll

 ;.
 define internal void @ptrarg.1(ptr %arg, i32 %val) argmemonly nounwind {
-; CHECK: Function Attrs: nounwind memory(readwrite, inaccessiblemem: none)
+; CHECK: Function Attrs: nounwind memory(readwrite, inaccessiblemem: none, errnomem: none)


I wonder if we should be setting errnomem as well in

llvm-project/llvm/lib/Transforms/IPO/SCCP.cpp

Lines 194 to 195 in 64735ad

ME |= MemoryEffects(IRMemLocation::Other,

ME.getModRef(IRMemLocation::ArgMem));

. The propagated global might be errno.

That's true, added this as well.

jdoerfert · 2025-02-11T19:43:13Z

There is no more deduction in this commit, right? And no propagation tests either, correct?

antoniofrighetto · 2025-02-11T20:18:14Z

There is no more deduction in this commit, right?

Right, will be introduced in a followup PR.

llvm/include/llvm/Support/ModRef.h

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

nikic

LGTM

llvm/include/llvm/Support/ModRef.h

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

Model C/C++ `errno` macro by adding a corresponding `errno` memory location kind to the IR. Preliminary work to separate `errno` writes from other memory accesses, to the benefit of alias analyses and optimization correctness. Previous discussion: https://discourse.llvm.org/t/rfc-modelling-errno-memory-effects/82972.

antoniofrighetto requested review from nikic, jdoerfert, dtcxzyw and efriedma-quic December 20, 2024 18:40

llvmbot added llvm:support llvm:ir llvm:transforms labels Dec 20, 2024

llvmbot added clang Clang issues not falling into any other category mlir:llvm mlir labels Dec 23, 2024

efriedma-quic reviewed Jan 10, 2025

View reviewed changes

antoniofrighetto force-pushed the feature/introduce-errno-loc branch from 54163c3 to 54d3ac9 Compare January 20, 2025 17:19

antoniofrighetto mentioned this pull request Jan 28, 2025

[InferAttrs] Mark errnomem-setting libcalls as such #124742

Open

antoniofrighetto force-pushed the feature/introduce-errno-loc branch from 98db3f9 to 196421e Compare February 4, 2025 17:26

antoniofrighetto force-pushed the feature/introduce-errno-loc branch from 196421e to 8d64748 Compare February 4, 2025 17:36

llvmbot added the llvm:analysis label Feb 5, 2025

antoniofrighetto force-pushed the feature/introduce-errno-loc branch from 6e799e9 to bf42779 Compare February 5, 2025 22:46

nikic reviewed Feb 6, 2025

View reviewed changes

antoniofrighetto force-pushed the feature/introduce-errno-loc branch from bf42779 to 93d21e2 Compare February 10, 2025 15:37

nikic reviewed Feb 10, 2025

View reviewed changes

nikic reviewed Feb 11, 2025

View reviewed changes

antoniofrighetto force-pushed the feature/introduce-errno-loc branch 2 times, most recently from 81fcb7f to 3adea2c Compare February 11, 2025 18:28

nikic reviewed Feb 12, 2025

View reviewed changes

llvm/include/llvm/Support/ModRef.h Outdated Show resolved Hide resolved

llvm/lib/Bitcode/Reader/BitcodeReader.cpp Outdated Show resolved Hide resolved

nikic approved these changes Feb 13, 2025

View reviewed changes

llvm/include/llvm/Support/ModRef.h Outdated Show resolved Hide resolved

llvm/lib/Bitcode/Reader/BitcodeReader.cpp Outdated Show resolved Hide resolved

antoniofrighetto force-pushed the feature/introduce-errno-loc branch from d56803f to fdf0104 Compare February 13, 2025 11:09

antoniofrighetto force-pushed the feature/introduce-errno-loc branch from fdf0104 to ff585fe Compare February 13, 2025 11:13

antoniofrighetto merged commit ff585fe into llvm:main Feb 13, 2025
6 of 8 checks passed

	else if (Kind == Attribute::Memory)
	B.addMemoryAttr(MemoryEffects::createFromIntValue(Record[++i]));

	ME \|= MemoryEffects(IRMemLocation::Other,
	ME.getModRef(IRMemLocation::ArgMem));

[IR][ModRef] Introduce errno memory location #120783

[IR][ModRef] Introduce errno memory location #120783

Conversation

antoniofrighetto commented Dec 20, 2024

llvmbot commented Dec 20, 2024 • edited Loading

antoniofrighetto commented Jan 9, 2025

efriedma-quic 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

antoniofrighetto Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2025 • edited Loading

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

antoniofrighetto Feb 10, 2025 • edited Loading

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 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

jdoerfert commented Feb 11, 2025

antoniofrighetto commented Feb 11, 2025

nikic left a comment

Choose a reason for hiding this comment

[IR][ModRef] Introduce `errno` memory location #120783

[IR][ModRef] Introduce `errno` memory location #120783

llvmbot commented Dec 20, 2024 •

edited

Loading

antoniofrighetto Feb 5, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

antoniofrighetto Feb 10, 2025 •

edited

Loading