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

[rahmanl]

Summary

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.

[rahmanl]

clang-format.

[rahmanl]

cleanup.

[rahmanl]

WIP.

[rahmanl]

Rebase and add comments.

[rahmanl]

clang-format.

[rahmanl]

clang-format.

[rahmanl]

Remove old accessors.

[rahmanl]

Use a bitwise equality operator, since memcmp won't work when padding is involved.

[rahmanl]

clang-format.

[rahmanl]

• cleanup.

[jhenderson]

llvm/include/llvm/Object/ELFTypes.h

820

Nit: use static_cast<unsigned>

822

Nit: blank line after method.

824

Could you add some simple unit tests for the encode/decode methods, please? They should be trivial to write, but I think would just help ensure there are no surprises. I think it would be sufficient to cover all 4 fields set individually, and also all fields set and no fields set.

830

FWIW, I'd find it more readable if you declared the struct, and then declared the member on a separate line i.e:

struct Metadata {
  ...
};

Metadata MD;

832–835

I'm not sure these should be moved. Typically, the constructor comes before other methods, from what I've seen, and it would reduce the size of the diff.

[rahmanl]

Add unittest for encoding and decoding.

[rahmanl]

clang-format.

[rahmanl]

• clang-format.

[jhenderson]

llvm/include/llvm/Object/ELFTypes.h

807–811

Nit: I'd add blank lines either side of this method.

813

Thinking about it, it's probably more appropriate for this to return a fixed width type (i.e. uint32_t) and similarly for the casts to cast to that.

Strictly speaking, I don't think there's any guarantee that unsigned is 4 bytes in size, plus it expresses the intent a little better in my opinion.

821

As above, uint32_t would be more appropriate here.

llvm/unittests/Object/ELFTypesTest.cpp

66 ↗

(On Diff #516467)

It might be interesting to show what happens when you provide decode with a value that includes bits that aren't related to any of the properties. For example, 0xffffffff.

73 ↗

(On Diff #516467)

What's the reason for these two being static? It's not modifiable, so I don't think it benefits anything, since it's already function-local.

74 ↗

(On Diff #516467)

These two loops should use size_t since that's what size returns.

[rahmanl]

• Add error handling for invalid encoding.
• Add llvm-readobj test.

[rahmanl]

llvm/unittests/Object/ELFTypesTest.cpp

66 ↗

(On Diff #516467)

Good point. I added a check to enforce 1-to-1 encoding.

[rahmanl]

@jhenderson Thanks for reviewing. Please let me know if I have addressed your comments.

[rahmanl]

• clang-format.

[rahmanl]

• Add error handling for invalid encoding.
• Add llvm-readobj test.

[rahmanl]

@jhenderson Another friendly ping.

[jhenderson]

Apologies for the delay: high priority "regular" work, combined with time off of various descriptions means my reviewing has mostly taken a back seat. LGTM.

llvm/lib/Object/ELF.cpp

735

Comment needs updating.

[rahmanl]

Update the comment.

[rahmanl]

Thank you for the review.

llvm/include/llvm/Object/ELFTypes.h

struct BBAddrMap {
   uint64_t Addr; // Function address
   // Struct representing the BBAddrMap information for one basic block.
   struct BBEntry {
     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.
 
     // 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)){};
+    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 memcmp(this, &Other, sizeof(Metadata)) == 0;
+      }
+      unsigned encode() const {
+        return ((unsigned)HasReturn) | (HasTailCall << 1) | (IsEHPad << 2) |
+               (CanFallThrough << 3);
+      }
+      static Metadata decode(unsigned V) {
+        return Metadata{
+            static_cast<bool>(V & 1), static_cast<bool>(V & (1 << 1)),
+            static_cast<bool>(V & (1 << 2)), static_cast<bool>(V & (1 << 3))};
+      }
+    } MD;
+
+    bool hasReturn() const { return MD.HasReturn; }
+    bool hasTailCall() const { return MD.HasTailCall; }
+    bool isEHPad() const { return MD.IsEHPad; }
+    bool canFallThrough() const { return MD.CanFallThrough; }
+
+    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;
+      return memcmp(this, &Other, sizeof(BBEntry)) == 0;
     }
   };
   std::vector<BBEntry> BBEntries; // Basic block entries for this function.
 
   // Equality operator for unit testing.
   bool operator==(const BBAddrMap &Other) const {
     return Addr == Other.Addr && std::equal(BBEntries.begin(), BBEntries.end(),
                                             Other.BBEntries.begin());
   }
 };
 
 } // end namespace object.
 } // end namespace llvm.
 
 #endif // LLVM_OBJECT_ELFTYPES_H

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCSymbolELF.h"
 #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"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/MathExtras.h"

 /// 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) {
   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()),
+      const_cast<MachineBasicBlock &>(MBB).canFallThrough(),
+      !MBB.empty() && MBB.rbegin()->isIndirectBranch()}
+      .encode();
 }
 
 void AsmPrinter::emitBBAddrMapSection(const MachineFunction &MF) {
   MCSection *BBAddrMapSection =
       getObjFileLowering().getBBAddrMapSection(*MF.getSection());
   assert(BBAddrMapSection && ".llvm_bb_addr_map section is not initialized.");
 
   const MCSymbol *FunctionSymbol = getFunctionBegin();

llvm/lib/Object/ELF.cpp

while (!ULEBSizeErr && Cur && Cur.tell() < Content.size()) {
     uint32_t NumBlocks = ReadULEB128AsUInt32();
     std::vector<BBAddrMap::BBEntry> BBEntries;
     uint32_t PrevBBEndOffset = 0;
     for (uint32_t BlockIndex = 0;
          !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});
+      BBEntries.push_back(
+          {ID, Offset, Size, BBAddrMap::BBEntry::Metadata::decode(MD)});
     }
     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));
   return FunctionEntries;
 }
 
 template <class ELFT>
 Expected<

llvm/tools/llvm-readobj/ELFDumper.cpp

for (const BBAddrMap &AM : *BBAddrMapOrErr) {
       W.printString("Name", FuncName);
 
       ListScope L(W, "BB entries");
       for (const BBAddrMap::BBEntry &BBE : AM.BBEntries) {
         DictScope L(W);
         W.printNumber("ID", BBE.ID);
         W.printHex("Offset", BBE.Offset);
         W.printHex("Size", BBE.Size);
-        W.printBoolean("HasReturn", BBE.HasReturn);
-        W.printBoolean("HasTailCall", BBE.HasTailCall);
-        W.printBoolean("IsEHPad", BBE.IsEHPad);
-        W.printBoolean("CanFallThrough", BBE.CanFallThrough);
+        W.printBoolean("HasReturn", BBE.hasReturn());
+        W.printBoolean("HasTailCall", BBE.hasTailCall());
+        W.printBoolean("IsEHPad", BBE.isEHPad());
+        W.printBoolean("CanFallThrough", BBE.canFallThrough());
       }
     }
   }
 }
 
 template <class ELFT> void LLVMELFDumper<ELFT>::printAddrsig() {
   ListScope L(W, "Addrsig");
   if (!this->DotAddrsigSec)

llvm/unittests/Object/ELFObjectFileTest.cpp

Entries:
         Address: 0x44444
         BBEntries:
           - ID:            0
             AddressOffset: 0x0
             Size:          0x4
             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};
   std::vector<BBAddrMap> Section2BBAddrMaps = {E1, E2};
   std::vector<BBAddrMap> AllBBAddrMaps = {E1, E2, E3, E4};
 
   auto DoCheckSucceeds = [&](StringRef YamlString,
                              std::optional<unsigned> TextSectionIndex,