[PATCH] D95851: Support for ELF section groups

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I prototyped something like this on an asm/linker level only (i.e. not the IR stuff), as part of some downstream experimentation work I did several years ago. This doesn't look all that dissimilar, which is reassuring, and probably suggests your on the right path to me.

+1 to splitting up the patch into separate patches. I think an LLD patch, an LLVM IR patch, and an assembler patch would be sufficient. I don't have any real expertees in the LLVM IR area, so can't be relied on to review that bit!

Re. how to indicate this in IR, I think a `group` tag is probably the way forawrd, which I think is what you've done here. You could say something like "`group` == in a regular group", "`comdat` == in a comdat group", "`group comdat` == in a comdat group". In other words, `comdat` on its own implies `group`. You wouldn't then need a verification check to show people haven't included them both. I don't know how practical this is however.



================
Comment at: llvm/lib/IR/Verifier.cpp:589
+    Assert(!(GO->hasGroup() && GO->hasComdat()),
+           "cannot have both a group and be in a comdat!", GO);
+
----------------
I guess to me a section is either "in a group" (not has a group), or in a comdat (which is a special kind of group), so perhaps the first bit of this message should be tweaked. Possibly we could add "non-comdat" before "group" to emphasise the difference, and maybe even add "group" after "comdat" too.


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

https://reviews.llvm.org/D95851