[PATCH] D153215: [Object] Add ELF section type SHT_LLVM_LTO for fat LTO

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:1039
   SHT_LLVM_OFFLOADING = 0x6fff4c0b,         // LLVM device offloading data.
+  SHT_LLVM_LTO = 0x6fff4c0c,                // .llvm.lto for fat LTO.
   // Android's experimental support for SHT_RELR sections.
----------------
MaskRay wrote:
> paulkirth wrote:
> > jhenderson wrote:
> > > paulkirth wrote:
> > > > jhenderson wrote:
> > > > > Perhaps `SHT_LLVM_FAT_LTO` so that it is a little more self-descriptive? (Equivalent comment applies to the section name and similar bits of data)
> > > > While this is for FatLTO now, I don't see why the section flag needs that distinction, since its a bit more generic (i.e., it's bitcode for LTO whether you use it for FatLTO or not). I don't have super strong feelings here, but I don't see much value in labeling it differently.
> > > Fair point. In which case, `SHT_LLVM_BITCODE` seems more self-descriptive (describing what it is rather than one potential use for it)? I imagine you could do things other than LTO with it in the future too after all :)
> > > 
> > > I'm not necessarily opposed to `SHT_LLVM_LTO`, but as this name is going to live a very long time, it's important we get it right!
> > > Fair point. In which case, `SHT_LLVM_BITCODE` seems more self-descriptive (describing what it is rather than one potential use for it)?
> > 
> > Those are all good points, but I think we want to describe that it's bitcode that is LTO ready. Using `.llvmbc` code is problematic for LTO use, so I could easily imagine that if this is `SHT_LLVM_BITCODE` that would lead to confusion or to the `.lvmbc` section being marked that way...
> > 
> > >I imagine you could do things other than LTO with it in the future too after all :)
> > > 
> >  Yeah, it sounds like a mismatch to imagine using the section for analysis if you're not doing LTO.
> > 
> > > I'm not necessarily opposed to `SHT_LLVM_LTO`, but as this name is going to live a very long time, it's important we get it right!
> > 
> > Naming is hard :(
> > 
> > Regardless of what we arrive at, lets be sure we document what its intended uses are, and why we chose that. As long as we do that, I think it will be fine, so I'm not going to oppose alternate names.
> One thought is to assign `SHT_LLVM_BITCODE` to both `.llvm.lto` and `.llvmbc`. We may need a function similar to `IRObjectFile::findBitcodeInMemBuffer` (in D146776) that picks a  `SHT_LLVM_BITCODE` section only when it is not called `.llvmbc`, i.e.
> 
> `type == SHT_LLVM_BITCODE && name != ".llvmbc"`
> Those are all good points, but I think we want to describe that it's bitcode that is LTO ready

I wasn't aware that there was a difference between different kinds of bitcode - I've never really looked into how bitcode and LTO work. Given this point, I'm okay with either `SHT_LLVM_LTO` or `SHT_LLVM_BITCODE`, depending on whether we go with @MaskRay's suggestion above. We could also try e.g. `SHT_LLVM_LTO_INPUT` maybe? But that might be starting to get a bit long.

> Naming is hard :(

Amen to that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153215