[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];
           }
----------------
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. ;)


================
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:
> 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;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104671