nickdesaulniers added a comment. In D68101#1765665 <https://reviews.llvm.org/D68101#1765665>, @rjmccall wrote:
> Given all that, this patch seems far too aggressive. While mergeable > sections can be useful for optimizing arbitrary code that might not use a > section, they are also extremely useful for optimizing the sorts of global > tables that programmers frequently use explicit sections for. It seems to me > that the right fix is to find the place that ensures that we don't put > mergeable and non-mergeable objects in the same section unit (or at least > conservatively makes the section unit non-mergeable) and fix it to consider > entry size as well. That should be straightforward unless that place doesn't > exist, in which case we have very serious problems. Disagree (but I spent all day thinking about this). Going back to https://bugs.llvm.org/show_bug.cgi?id=31828, I think I solved the problem there incorrectly; we should be not marking the explicit section merge-able. Consider: https://godbolt.org/z/6sF_kc (GCC will put `a` and `c` in a non-mergeable section). https://www.sco.com/developers/gabi/2003-12-17/ch4.sheader.html mentions: Each element in the section is compared against other elements in sections with the same name, type and flags. Elements that would have identical values at program run-time may be merged. Relocations referencing elements of such sections must be resolved to the merged locations of the referenced values. Note that any relocatable values, including values that would result in run-time relocations, must be analyzed to determine whether the run-time values would actually be identical. So I don't think we should even be trying to mark sections as mergeable unless we walk all elements in the section and make sure it's even safe to apply these. In D68101#1684828 <https://reviews.llvm.org/D68101#1684828>, @jmolloy wrote: > This feels like it could cause a pretty serious regression. This essentially > disables global merging with -fdata-sections, which I know at least one > linker relies upon for code size. Does it? Surely there's a test that would fail with this patch applied? ================ Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:21 +; CHECK: explicit2: +; CHECK-NOT: .section +; CHECK: explicit3: ---------------- These `CHECK-NOT`s should be more explicit that the the section is not merge-able (`M`). ================ Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:22 +; CHECK-NOT: .section +; CHECK: explicit3: ---------------- Any new tests should be added to `llvm/test/CodeGen/Generic/section_mergeable_size.ll` IMO. I assume this test changes that test's behavior? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68101/new/ https://reviews.llvm.org/D68101 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
