[PATCH] D104671: [lld-macho] Extend ICF to literal sections

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/ICF.cpp:112-113
             const auto *isecB = cast<ConcatInputSection>(db->isec);
-            if (isecA->icfEqClass[icfPass % 2] !=
-                isecB->icfEqClass[icfPass % 2])
-              return false;
-          } else {
-            // FIXME: implement ICF for other InputSection kinds
-            return false;
+            return da->value == db->value && isecA->icfEqClass[icfPass % 2] ==
+                                                 isecB->icfEqClass[icfPass % 2];
           }
----------------
int3 wrote:
> gkm wrote:
> > int3 wrote:
> > > gkm wrote:
> > > > I prefer this for convenient breakpoint locations when particular conditions fail.
> > > it feels unfortunate to contort the code for easier breakpointing. Is the goal to be able to breakpoint right before function exit? for that case, could we just set a breakpoint at the `retq` instruction's address? (I checked the disassembly, and clang does indeed emit a single retq for this whole function)
> > IMO, it's not contortion so much as vertical expansion! ;)
> > I want to see if a particular predicate fails, so I'll breakpoint the `return false;` beneath it.
> hm, so the idea is that you're more likely to want to debug when things don't compare to be equal?
> IMO, it's not contortion so much as vertical expansion! ;)

I think it makes for more confusing code, because it obscures when fall-through behavior (to the `return true;`) is actually needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104671