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

[Greg McGary via Phabricator via llvm-commits]

gkm 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];
           }
----------------
gkm wrote:
> gkm wrote:
> > int3 wrote:
> > > 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.
> > > hm, so the idea is that you're more likely to want to debug when things don't compare to be equal?
> > 
> > So far, all of my debugging has been to see when predicates unexpectedly fail. We don't need to debate this any further. Do what you think best. ;)
> It makes sense when you recognize that everything follows a regular pattern: A sequence of `if (UNEQUAL) return false;` falling through to `return true;`
Overall, the `if (UNEQUAL) return false;` pattern serves all breakpointing purposes: when there is an unexpected predicate failure, I break on the `return false;`, and when there is an unexpected predicate success, I break on the statement following the entire `if` statement. I like it. It works for me. FYI, the earliest versions of `equalsConstant()` and `equalsVariable()` were beautiful single statements: `return MASSIVE-MULTILINE-COMPOUND-PREDICATE`, but then I needed to debug and wanted fine-grained control over breakpoints. ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104671