[PATCH] D128474: [BOLT] Support multiple parents for split jump table

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Thanks Huan for all the work put in this diff. I have a few more comments below.



================
Comment at: bolt/include/bolt/Core/BinaryContext.h:238-241
+  /// Return function fragments to skip.
+  const std::unordered_set<BinaryFunction *> &getFragmentsToSkip() {
+    return FragmentsToSkip;
+  }
----------------
Remove dead code


================
Comment at: bolt/lib/Core/BinaryContext.cpp:498
                                      const uint64_t NextJTAddress,
                                      JumpTable::OffsetsType *Offsets) {
   // Is one of the targets __builtin_unreachable?
----------------
I think it's a bit weird to keep calling this "Offsets", and "OffsetEntries" in JumpTable if we are not using offsets anymore, right? We are now using the full address in each entry. Perhaps we should rename this in JumpTable class? Something like "EntriesAsAddresses" in JumpTables.h:73 - 74 (and update comments there)


================
Comment at: bolt/lib/Core/BinaryContext.cpp:573
     if (Value == BF.getAddress() + BF.getSize()) {
-      addOffset(Value - BF.getAddress());
+      addOffset(Value);
       HasUnreachable = true;
----------------
Same renaming here (my motivation here is why are we calling this an offset if this is not an offset anymore)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128474/new/

https://reviews.llvm.org/D128474