address review feedback

Created using spr 1.3.7

Author: cachemeifyoucan

llvm/include/llvm/CAS/OnDiskGraphDB.h

@@ -22,7 +22,7 @@
 
 namespace llvm::cas::ondisk {
 
-/// Standard 8B reference inside OnDiskGraphDB.
+/// Standard 8 byte reference inside OnDiskGraphDB.
 class InternalRef {
 public:
   FileOffset getFileOffset() const { return FileOffset(Data); }
@@ -43,7 +43,7 @@ class InternalRef {
   uint64_t Data;
 };
 
-/// Compact 4B reference inside OnDiskGraphDB for smaller references.
+/// Compact 4 byte reference inside OnDiskGraphDB for smaller references.
 class InternalRef4B {
 public:
   FileOffset getFileOffset() const { return FileOffset(Data); }
@@ -199,9 +199,8 @@ class ObjectHandle {
 public:
   uint64_t getOpaqueData() const { return Opaque; }
 
-  static ObjectHandle fromOpaqueData(uint64_t Opaque) {
-    return ObjectHandle(Opaque);
-  }
+  static ObjectHandle fromFileOffset(FileOffset Offset);
+  static ObjectHandle fromMemory(uintptr_t Ptr);
 
   friend bool operator==(const ObjectHandle &LHS, const ObjectHandle &RHS) {
     return LHS.Opaque == RHS.Opaque;
@@ -401,8 +400,8 @@ class OnDiskGraphDB {
   /// Create a standalone leaf file.
   Error createStandaloneLeaf(IndexProxy &I, ArrayRef<char> Data);
 
-  /// @name Helper functions for internal data structures.
-  /// @{
+  /// \name Helper functions for internal data structures.
+  /// \{
   static InternalRef getInternalRef(ObjectID Ref) {
     return InternalRef::getFromRawData(Ref.getOpaqueData());
   }
@@ -425,7 +424,7 @@ class OnDiskGraphDB {
   getIndexProxyFromPointer(OnDiskTrieRawHashMap::ConstOnDiskPtr P) const;
 
   InternalRefArrayRef getInternalRefs(ObjectHandle Node) const;
-  /// @}
+  /// \}
 
   /// Get the atomic variable that keeps track of the standalone data storage
   /// size.
@@ -453,7 +452,7 @@ class OnDiskGraphDB {
   OnDiskDataAllocator DataPool;
 
   /// A StandaloneDataMap.
-  void *StandaloneData;
+  void *StandaloneData = nullptr;
 
   /// Path to the root directory.
   std::string RootPath;

llvm/lib/CAS/OnDiskGraphDB.cpp

@@ -13,7 +13,7 @@
 ///
 /// OnDiskGraphDB defines:
 ///
-/// - How the data are stored inside database, either as a standalone file, or
+/// - How the data is stored inside database, either as a standalone file, or
 ///   allocated inside a datapool.
 /// - How references to other objects inside the same database is stored. They
 ///   are stored as internal references, instead of full hash value to save
@@ -90,7 +90,7 @@ static Error createCorruptObjectError(Expected<ArrayRef<uint8_t>> ID) {
 
 namespace {
 
-/// Trie record data: 8B, atomic<uint64_t>
+/// Trie record data: 8 bytes, atomic<uint64_t>
 /// - 1-byte: StorageKind
 /// - 7-bytes: DataStoreOffset (offset into referenced file)
 class TrieRecord {
@@ -129,7 +129,7 @@ class TrieRecord {
   }
 
   enum Limits : int64_t {
-    // Saves files bigger than 64KB standalone instead of embedding them.
+    /// Saves files bigger than 64KB standalone instead of embedding them.
     MaxEmbeddedSize = 64LL * 1024LL - 1,
   };
 
@@ -138,6 +138,7 @@ class TrieRecord {
     FileOffset Offset;
   };
 
+  /// Pack StorageKind and Offset from Data into 8 byte TrieRecord.
   static uint64_t pack(Data D) {
     assert(D.Offset.get() < (int64_t)(1ULL << 56));
     uint64_t Packed = uint64_t(D.SK) << 56 | D.Offset.get();
@@ -150,6 +151,7 @@ class TrieRecord {
     return Packed;
   }
 
+  // Unpack TrieRecord into Data.
   static Data unpack(uint64_t Packed) {
     Data D;
     if (!Packed)
@@ -221,7 +223,7 @@ struct DataRecordHandle {
                     0,
                 "Not enough bits");
 
-  // layout of the DataRecordHandle and how to decode it.
+  /// Layout of the DataRecordHandle and how to decode it.
   struct LayoutFlags {
     NumRefsFlags NumRefs;
     DataSizeFlags DataSize;
@@ -287,10 +289,12 @@ struct DataRecordHandle {
     return getDataRelOffset() + getDataSize() + 1;
   }
 
+  /// Describe the layout of data stored and how to decode from
+  /// DataRecordHandle.
   struct Layout {
     explicit Layout(const Input &I);
 
-    LayoutFlags Flags{};
+    LayoutFlags Flags;
     uint64_t DataSize = 0;
     uint32_t NumRefs = 0;
     int64_t RefsRelOffset = 0;
@@ -349,7 +353,7 @@ struct OnDiskContent {
   std::optional<ArrayRef<char>> Bytes;
 };
 
-// Data loaded inside the memory from standalone file.
+/// Data loaded inside the memory from standalone file.
 class StandaloneDataInMemory {
 public:
   OnDiskContent getContent() const;
@@ -382,9 +386,8 @@ template <size_t NumShards> class StandaloneDataMap {
   static_assert(isPowerOf2_64(NumShards), "Expected power of 2");
 
 public:
-  const StandaloneDataInMemory &
-  insert(ArrayRef<uint8_t> Hash, TrieRecord::StorageKind SK,
-         std::unique_ptr<sys::fs::mapped_file_region> Region);
+  uintptr_t insert(ArrayRef<uint8_t> Hash, TrieRecord::StorageKind SK,
+                   std::unique_ptr<sys::fs::mapped_file_region> Region);
 
   const StandaloneDataInMemory *lookup(ArrayRef<uint8_t> Hash) const;
   bool count(ArrayRef<uint8_t> Hash) const { return bool(lookup(Hash)); }
@@ -409,27 +412,7 @@ template <size_t NumShards> class StandaloneDataMap {
 
 using StandaloneDataMapTy = StandaloneDataMap<16>;
 
-struct InternalHandle {
-  FileOffset getAsFileOffset() const { return *DataOffset; }
-
-  uint64_t getRawData() const {
-    if (DataOffset) {
-      uint64_t Raw = DataOffset->get();
-      assert(!(Raw & 0x1));
-      return Raw;
-    }
-    uint64_t Raw = reinterpret_cast<uintptr_t>(SDIM);
-    assert(!(Raw & 0x1));
-    return Raw | 1;
-  }
-
-  explicit InternalHandle(FileOffset DataOffset) : DataOffset(DataOffset) {}
-  explicit InternalHandle(uint64_t DataOffset) : DataOffset(DataOffset) {}
-  explicit InternalHandle(const StandaloneDataInMemory &SDIM) : SDIM(&SDIM) {}
-  std::optional<FileOffset> DataOffset;
-  const StandaloneDataInMemory *SDIM = nullptr;
-};
-
+/// A vector of internal node references.
 class InternalRefVector {
 public:
   void push_back(InternalRef Ref) {
@@ -476,6 +459,18 @@ DataRecordHandle::create(function_ref<char *(size_t Size)> Alloc,
   return constructImpl(Alloc(L.getTotalSize()), I, L);
 }
 
+ObjectHandle ObjectHandle::fromFileOffset(FileOffset Offset) {
+  // Store the file offset as it is.
+  assert(!(Offset.get() & 0x1));
+  return ObjectHandle(Offset.get());
+}
+
+ObjectHandle ObjectHandle::fromMemory(uintptr_t Ptr) {
+  // Store the pointer from memory with lowest bit set.
+  assert(!(Ptr & 0x1));
+  return ObjectHandle(Ptr | 1);
+}
+
 /// Proxy for an on-disk index record.
 struct OnDiskGraphDB::IndexProxy {
   FileOffset Offset;
@@ -484,15 +479,15 @@ struct OnDiskGraphDB::IndexProxy {
 };
 
 template <size_t N>
-const StandaloneDataInMemory &StandaloneDataMap<N>::insert(
+uintptr_t StandaloneDataMap<N>::insert(
     ArrayRef<uint8_t> Hash, TrieRecord::StorageKind SK,
     std::unique_ptr<sys::fs::mapped_file_region> Region) {
   auto &S = getShard(Hash);
   std::lock_guard<std::mutex> Lock(S.Mutex);
   auto &V = S.Map[Hash.data()];
   if (!V)
     V = std::make_unique<StandaloneDataInMemory>(std::move(Region), SK);
-  return *V;
+  return reinterpret_cast<uintptr_t>(V.get());
 }
 
 template <size_t N>
@@ -1168,20 +1163,16 @@ ArrayRef<uint8_t> OnDiskGraphDB::getDigest(const IndexProxy &I) const {
 
 static OnDiskContent getContentFromHandle(const OnDiskDataAllocator &DataPool,
                                           ObjectHandle OH) {
-  auto getInternalHandle = [](ObjectHandle Handle) -> InternalHandle {
-    uint64_t Data = Handle.getOpaqueData();
-    if (Data & 1)
-      return InternalHandle(*reinterpret_cast<const StandaloneDataInMemory *>(
-          Data & (-1ULL << 1)));
-    return InternalHandle(Data);
-  };
-
-  InternalHandle Handle = getInternalHandle(OH);
-  if (Handle.SDIM)
-    return Handle.SDIM->getContent();
+  // Decode ObjectHandle to locate the stored content.
+  uint64_t Data = OH.getOpaqueData();
+  if (Data & 1) {
+    const auto *SDIM =
+        reinterpret_cast<const StandaloneDataInMemory *>(Data & (-1ULL << 1));
+    return SDIM->getContent();
+  }
 
-  auto DataHandle = cantFail(
-      DataRecordHandle::getFromDataPool(DataPool, Handle.getAsFileOffset()));
+  auto DataHandle =
+      cantFail(DataRecordHandle::getFromDataPool(DataPool, FileOffset(Data)));
   assert(DataHandle.getData().end()[0] == 0 && "Null termination");
   return OnDiskContent{DataHandle, std::nullopt};
 }
@@ -1215,12 +1206,8 @@ OnDiskGraphDB::load(ObjectID ExternalRef) {
     return faultInFromUpstream(ExternalRef);
   }
 
-  auto toObjectHandle = [](InternalHandle H) -> ObjectHandle {
-    return ObjectHandle::fromOpaqueData(H.getRawData());
-  };
-
   if (Object.SK == TrieRecord::StorageKind::DataPool)
-    return toObjectHandle(InternalHandle(Object.Offset));
+    return ObjectHandle::fromFileOffset(Object.Offset);
 
   // Only TrieRecord::StorageKind::Standalone (and variants) need to be
   // explicitly loaded.
@@ -1263,9 +1250,9 @@ OnDiskGraphDB::load(ObjectID ExternalRef) {
   if (EC)
     return createCorruptObjectError(getDigest(*I));
 
-  return toObjectHandle(
-      InternalHandle(static_cast<StandaloneDataMapTy *>(StandaloneData)
-                         ->insert(I->Hash, Object.SK, std::move(Region))));
+  return ObjectHandle::fromMemory(
+      static_cast<StandaloneDataMapTy *>(StandaloneData)
+          ->insert(I->Hash, Object.SK, std::move(Region)));
 }
 
 Expected<bool> OnDiskGraphDB::isMaterialized(ObjectID Ref) {

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

@@ -2979,10 +2979,14 @@ Instruction *InstCombinerImpl::foldAndOrOfSelectUsingImpliedCond(Value *Op,
          "Op must be either i1 or vector of i1.");
   if (SI.getCondition()->getType() != Op->getType())
     return nullptr;
-  if (Value *V = simplifyNestedSelectsUsingImpliedCond(SI, Op, IsAnd, DL))
-    return SelectInst::Create(Op,
-                              IsAnd ? V : ConstantInt::getTrue(Op->getType()),
-                              IsAnd ? ConstantInt::getFalse(Op->getType()) : V);
+  if (Value *V = simplifyNestedSelectsUsingImpliedCond(SI, Op, IsAnd, DL)) {
+    Instruction *MDFrom = nullptr;
+    if (!ProfcheckDisableMetadataFixes)
+      MDFrom = &SI;
+    return SelectInst::Create(
+        Op, IsAnd ? V : ConstantInt::getTrue(Op->getType()),
+        IsAnd ? ConstantInt::getFalse(Op->getType()) : V, "", nullptr, MDFrom);
+  }
   return nullptr;
 }
 

llvm/test/Transforms/InstCombine/select-safe-impliedcond-transforms.ll

@@ -1,4 +1,4 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals
 ; RUN: opt < %s -passes=instcombine -S | FileCheck %s
 
 define i1 @a_true_implies_b_true(i8 %z, i1 %X, i1 %Y) {
@@ -34,15 +34,15 @@ define <2 x i1> @a_true_implies_b_true_vec(i8 %z0, <2 x i1> %X, <2 x i1> %Y) {
   ret <2 x i1> %res
 }
 
-define i1 @a_true_implies_b_true2(i8 %z, i1 %X, i1 %Y) {
+define i1 @a_true_implies_b_true2(i8 %z, i1 %X, i1 %Y) !prof !0 {
 ; CHECK-LABEL: @a_true_implies_b_true2(
 ; CHECK-NEXT:    [[A:%.*]] = icmp ugt i8 [[Z:%.*]], 20
-; CHECK-NEXT:    [[RES:%.*]] = select i1 [[A]], i1 [[X:%.*]], i1 false
+; CHECK-NEXT:    [[RES:%.*]] = select i1 [[A]], i1 [[X:%.*]], i1 false, !prof [[PROF1:![0-9]+]]
 ; CHECK-NEXT:    ret i1 [[RES]]
 ;
   %a = icmp ugt i8 %z, 20
   %b = icmp ugt i8 %z, 10
-  %sel = select i1 %b, i1 %X, i1 %Y
+  %sel = select i1 %b, i1 %X, i1 %Y, !prof !1
   %res = and i1 %a, %sel
   ret i1 %res
 }
@@ -258,3 +258,10 @@ define i1 @neg_icmp_eq_implies_trunc(i8 %x, i1 %c) {
   %sel2 = select i1 %cmp, i1 true, i1 %sel1
   ret i1 %sel2
 }
+
+!0 = !{!"function_entry_count", i64 1000}
+!1 = !{!"branch_weights", i32 2, i32 3}
+;.
+; CHECK: [[META0:![0-9]+]] = !{!"function_entry_count", i64 1000}
+; CHECK: [[PROF1]] = !{!"branch_weights", i32 2, i32 3}
+;.

llvm/utils/profcheck-xfail.txt

@@ -906,7 +906,6 @@ Transforms/InstCombine/select_frexp.ll
 Transforms/InstCombine/select.ll
 Transforms/InstCombine/select-min-max.ll
 Transforms/InstCombine/select-of-symmetric-selects.ll
-Transforms/InstCombine/select-safe-impliedcond-transforms.ll
 Transforms/InstCombine/select-safe-transforms.ll
 Transforms/InstCombine/select-select.ll
 Transforms/InstCombine/select-with-extreme-eq-cond.ll
@@ -1237,7 +1236,6 @@ Transforms/PartiallyInlineLibCalls/X86/good-prototype.ll
 Transforms/PGOProfile/chr-dead-pred.ll
 Transforms/PGOProfile/chr-dup-threshold.ll
 Transforms/PGOProfile/chr-lifetimes.ll
-Transforms/PGOProfile/chr.ll
 Transforms/PGOProfile/chr-poison.ll
 Transforms/PGOProfile/comdat.ll
 Transforms/PGOProfile/memop_profile_funclet_wasm.ll