[Propeller] Use a bit-field struct for the metdata fields of BBEntry.

This patch encapsulates the encoding and decoding logic of basic block metadata into the Metadata struct, and also reduces the decoded size of `SHT_LLVM_BB_ADDR_MAP` section.

The patch would've looked more readable if we could use designated initializer, but that is a c++20 feature.

Reviewed By: jhenderson

Differential Revision: https://reviews.llvm.org/D148360

Author: rlavaee

llvm/include/llvm/Object/ELFTypes.h

@@ -799,32 +799,57 @@ struct BBAddrMap {
   uint64_t Addr; // Function address
   // Struct representing the BBAddrMap information for one basic block.
   struct BBEntry {
+    struct Metadata {
+      bool HasReturn : 1;   // If this block ends with a return (or tail call).
+      bool HasTailCall : 1; // If this block ends with a tail call.
+      bool IsEHPad : 1;     // If this is an exception handling block.
+      bool CanFallThrough : 1; // If this block can fall through to its next.
+
+      bool operator==(const Metadata &Other) const {
+        return HasReturn == Other.HasReturn &&
+               HasTailCall == Other.HasTailCall && IsEHPad == Other.IsEHPad &&
+               CanFallThrough == Other.CanFallThrough;
+      }
+
+      // Encodes this struct as a uint32_t value.
+      uint32_t encode() const {
+        return static_cast<uint32_t>(HasReturn) |
+               (static_cast<uint32_t>(HasTailCall) << 1) |
+               (static_cast<uint32_t>(IsEHPad) << 2) |
+               (static_cast<uint32_t>(CanFallThrough) << 3);
+      }
+
+      // Decodes and returns a Metadata struct from a uint32_t value.
+      static Expected<Metadata> decode(uint32_t V) {
+        Metadata MD{/*HasReturn=*/static_cast<bool>(V & 1),
+                    /*HasTailCall=*/static_cast<bool>(V & (1 << 1)),
+                    /*IsEHPad=*/static_cast<bool>(V & (1 << 2)),
+                    /*CanFallThrough=*/static_cast<bool>(V & (1 << 3))};
+        if (MD.encode() != V)
+          return createStringError(
+              std::error_code(), "invalid encoding for BBEntry::Metadata: 0x%x",
+              V);
+        return MD;
+      }
+    };
+
     uint32_t ID;     // Unique ID of this basic block.
     uint32_t Offset; // Offset of basic block relative to function start.
     uint32_t Size;   // Size of the basic block.
+    Metadata MD;     // Metdata for this basic block.
 
-    // The following fields are decoded from the Metadata field. The encoding
-    // happens in AsmPrinter.cpp:getBBAddrMapMetadata.
-    bool HasReturn;      // If this block ends with a return (or tail call).
-    bool HasTailCall;    // If this block ends with a tail call.
-    bool IsEHPad;        // If this is an exception handling block.
-    bool CanFallThrough; // If this block can fall through to its next.
-
-    BBEntry(uint32_t ID, uint32_t Offset, uint32_t Size, uint32_t Metadata)
-        : ID(ID), Offset(Offset), Size(Size), HasReturn(Metadata & 1),
-          HasTailCall(Metadata & (1 << 1)), IsEHPad(Metadata & (1 << 2)),
-          CanFallThrough(Metadata & (1 << 3)){};
+    BBEntry(uint32_t ID, uint32_t Offset, uint32_t Size, Metadata MD)
+        : ID(ID), Offset(Offset), Size(Size), MD(MD){};
 
     bool operator==(const BBEntry &Other) const {
       return ID == Other.ID && Offset == Other.Offset && Size == Other.Size &&
-             HasReturn == Other.HasReturn && HasTailCall == Other.HasTailCall &&
-             IsEHPad == Other.IsEHPad && CanFallThrough == Other.CanFallThrough;
+             MD == Other.MD;
     }
 
-    bool hasReturn() const { return HasReturn; }
-    bool hasTailCall() const { return HasTailCall; }
-    bool isEHPad() const { return IsEHPad; }
-    bool canFallThrough() const { return CanFallThrough; }
+    bool hasReturn() const { return MD.HasReturn; }
+    bool hasTailCall() const { return MD.HasTailCall; }
+    bool isEHPad() const { return MD.IsEHPad; }
+    bool canFallThrough() const { return MD.CanFallThrough; }
   };
   std::vector<BBEntry> BBEntries; // Basic block entries for this function.
 

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

@@ -99,6 +99,7 @@
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/MC/SectionKind.h"
+#include "llvm/Object/ELFTypes.h"
 #include "llvm/Pass.h"
 #include "llvm/Remarks/RemarkStreamer.h"
 #include "llvm/Support/Casting.h"
@@ -1330,21 +1331,15 @@ void AsmPrinter::emitFrameAlloc(const MachineInstr &MI) {
                              MCConstantExpr::create(FrameOffset, OutContext));
 }
 
-/// Returns the BB metadata to be emitted in the .llvm_bb_addr_map section for a
-/// given basic block. This can be used to capture more precise profile
-/// information. We use the last 4 bits (LSBs) to encode the following
-/// information:
-///  * (1): set if return block (ret or tail call).
-///  * (2): set if ends with a tail call.
-///  * (3): set if exception handling (EH) landing pad.
-///  * (4): set if the block can fall through to its next.
-/// The remaining bits are zero.
-static unsigned getBBAddrMapMetadata(const MachineBasicBlock &MBB) {
+/// Returns the BB metadata to be emitted in the SHT_LLVM_BB_ADDR_MAP section
+/// for a given basic block. This can be used to capture more precise profile
+/// information.
+static uint32_t getBBAddrMapMetadata(const MachineBasicBlock &MBB) {
   const TargetInstrInfo *TII = MBB.getParent()->getSubtarget().getInstrInfo();
-  return ((unsigned)MBB.isReturnBlock()) |
-         ((!MBB.empty() && TII->isTailCall(MBB.back())) << 1) |
-         (MBB.isEHPad() << 2) |
-         (const_cast<MachineBasicBlock &>(MBB).canFallThrough() << 3);
+  return object::BBAddrMap::BBEntry::Metadata{
+      MBB.isReturnBlock(), !MBB.empty() && TII->isTailCall(MBB.back()),
+      MBB.isEHPad(), const_cast<MachineBasicBlock &>(MBB).canFallThrough()}
+      .encode();
 }
 
 void AsmPrinter::emitBBAddrMapSection(const MachineFunction &MF) {

llvm/lib/Object/ELF.cpp

@@ -674,6 +674,7 @@ ELFFile<ELFT>::decodeBBAddrMap(const Elf_Shdr &Sec,
 
   DataExtractor::Cursor Cur(0);
   Error ULEBSizeErr = Error::success();
+  Error MetadataDecodeErr = Error::success();
   // Helper to extract and decode the next ULEB128 value as uint32_t.
   // Returns zero and sets ULEBSizeErr if the ULEB128 value exceeds the uint32_t
   // limit.
@@ -694,7 +695,8 @@ ELFFile<ELFT>::decodeBBAddrMap(const Elf_Shdr &Sec,
   };
 
   uint8_t Version = 0;
-  while (!ULEBSizeErr && Cur && Cur.tell() < Content.size()) {
+  while (!ULEBSizeErr && !MetadataDecodeErr && Cur &&
+         Cur.tell() < Content.size()) {
     if (Sec.sh_type == ELF::SHT_LLVM_BB_ADDR_MAP) {
       Version = Data.getU8(Cur);
       if (!Cur)
@@ -722,24 +724,32 @@ ELFFile<ELFT>::decodeBBAddrMap(const Elf_Shdr &Sec,
     std::vector<BBAddrMap::BBEntry> BBEntries;
     uint32_t PrevBBEndOffset = 0;
     for (uint32_t BlockIndex = 0;
-         !ULEBSizeErr && Cur && (BlockIndex < NumBlocks); ++BlockIndex) {
+         !MetadataDecodeErr && !ULEBSizeErr && Cur && (BlockIndex < NumBlocks);
+         ++BlockIndex) {
       uint32_t ID = Version >= 2 ? ReadULEB128AsUInt32() : BlockIndex;
       uint32_t Offset = ReadULEB128AsUInt32();
       uint32_t Size = ReadULEB128AsUInt32();
-      uint32_t Metadata = ReadULEB128AsUInt32();
+      uint32_t MD = ReadULEB128AsUInt32();
       if (Version >= 1) {
         // Offset is calculated relative to the end of the previous BB.
         Offset += PrevBBEndOffset;
         PrevBBEndOffset = Offset + Size;
       }
-      BBEntries.push_back({ID, Offset, Size, Metadata});
+      Expected<BBAddrMap::BBEntry::Metadata> MetadataOrErr =
+          BBAddrMap::BBEntry::Metadata::decode(MD);
+      if (!MetadataOrErr) {
+        MetadataDecodeErr = MetadataOrErr.takeError();
+        break;
+      }
+      BBEntries.push_back({ID, Offset, Size, *MetadataOrErr});
     }
     FunctionEntries.push_back({Address, std::move(BBEntries)});
   }
-  // Either Cur is in the error state, or ULEBSizeError is set (not both), but
-  // we join the two errors here to be safe.
-  if (!Cur || ULEBSizeErr)
-    return joinErrors(Cur.takeError(), std::move(ULEBSizeErr));
+  // Either Cur is in the error state, or we have an error in ULEBSizeErr or
+  // MetadataDecodeErr (but not both), but we join all errors here to be safe.
+  if (!Cur || ULEBSizeErr || MetadataDecodeErr)
+    return joinErrors(joinErrors(Cur.takeError(), std::move(ULEBSizeErr)),
+                      std::move(MetadataDecodeErr));
   return FunctionEntries;
 }
 

llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test

@@ -21,6 +21,10 @@
 # RUN: yaml2obj --docnum=1 %s -DBITS=32 -DSIZE=6 -o %t2.o
 # RUN: llvm-readobj %t2.o --bb-addr-map 2>&1 | FileCheck %s -DOFFSET=0x00000006 -DFILE=%t2.o --check-prefix=TRUNCATED
 
+## Check that invalid metadata can be handled.
+# RUN: yaml2obj --docnum=1 %s -DBITS=32 -DMETADATA=0xF000002 -o %t3.o
+# RUN: llvm-readobj %t3.o --bb-addr-map 2>&1 | FileCheck %s -DFILE=%t3.o --check-prefix=INVALIDMD
+
 # CHECK:      BBAddrMap [
 # CHECK-NEXT:   Function {
 # CHECK-NEXT:     At: [[ADDR]]
@@ -97,6 +101,9 @@
 # TRUNCATED-NEXT:   }
 # TRUNCATED-NEXT: ]
 
+# INVALIDMD:      BBAddrMap [
+# INVALIDMD-NEXT:   warning: '[[FILE]]': unable to dump SHT_LLVM_BB_ADDR_MAP section with index 3: invalid encoding for BBEntry::Metadata: 0xf000002
+
 --- !ELF
 FileHeader:
   Class: ELFCLASS[[BITS]]
@@ -120,7 +127,7 @@ Sections:
           - ID:            0
             AddressOffset: 0x0
             Size:          0x1
-            Metadata:      0xF0000002
+            Metadata:      [[METADATA=0x2]]
           - ID:            2
             AddressOffset: 0x3
             Size:          0x4

llvm/unittests/Object/ELFObjectFileTest.cpp

@@ -661,10 +661,10 @@ TEST(ELFObjectFileTest, ReadBBAddrMap) {
             Metadata:      0x8
 )");
 
-  BBAddrMap E1 = {0x11111, {{1, 0x0, 0x1, 0x2}}};
-  BBAddrMap E2 = {0x22222, {{2, 0x0, 0x2, 0x4}}};
-  BBAddrMap E3 = {0x33333, {{0, 0x0, 0x3, 0x6}}};
-  BBAddrMap E4 = {0x44444, {{0, 0x0, 0x4, 0x8}}};
+  BBAddrMap E1 = {0x11111, {{1, 0x0, 0x1, {false, true, false, false}}}};
+  BBAddrMap E2 = {0x22222, {{2, 0x0, 0x2, {false, false, true, false}}}};
+  BBAddrMap E3 = {0x33333, {{0, 0x0, 0x3, {false, true, true, false}}}};
+  BBAddrMap E4 = {0x44444, {{0, 0x0, 0x4, {false, false, false, true}}}};
 
   std::vector<BBAddrMap> Section0BBAddrMaps = {E4};
   std::vector<BBAddrMap> Section1BBAddrMaps = {E3};

llvm/unittests/Object/ELFTypesTest.cpp

@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 #include "llvm/Object/ELFTypes.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 #include <iostream>
 
@@ -61,3 +62,34 @@ TEST(ELFTypesTest, NoteTest) {
       TestData.getElfNote("AMD", ELF::NT_AMDGPU_METADATA, ArrayRef<uint8_t>());
   EXPECT_EQ(Note3.getDescAsStringRef(4), StringRef(""));
 }
+
+TEST(ELFTypesTest, BBEntryMetadataEncodingTest) {
+  const std::array<BBAddrMap::BBEntry::Metadata, 6> Decoded = {
+      {{false, false, false, false},
+       {true, false, false, false},
+       {false, true, false, false},
+       {false, false, true, false},
+       {false, false, false, true},
+       {true, true, true, true}}};
+  const std::array<uint32_t, 6> Encoded = {{0, 1, 2, 4, 8, 15}};
+  for (size_t i = 0; i < Decoded.size(); ++i)
+    EXPECT_EQ(Decoded[i].encode(), Encoded[i]);
+  for (size_t i = 0; i < Encoded.size(); ++i) {
+    Expected<BBAddrMap::BBEntry::Metadata> MetadataOrError =
+        BBAddrMap::BBEntry::Metadata::decode(Encoded[i]);
+    ASSERT_THAT_EXPECTED(MetadataOrError, Succeeded());
+    EXPECT_EQ(*MetadataOrError, Decoded[i]);
+  }
+}
+
+TEST(ELFTypesTest, BBEntryMetadataInvalidEncodingTest) {
+  const std::array<std::string, 2> Errors = {
+      "invalid encoding for BBEntry::Metadata: 0xffff",
+      "invalid encoding for BBEntry::Metadata: 0x100001"};
+  const std::array<uint32_t, 2> Values = {{0xFFFF, 0x100001}};
+  for (size_t i = 0; i < Values.size(); ++i) {
+    EXPECT_THAT_ERROR(
+        BBAddrMap::BBEntry::Metadata::decode(Values[i]).takeError(),
+        FailedWithMessage(Errors[i]));
+  }
+}