This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Support COMDAT sections in assembly syntax
ClosedPublic

Authored by dschuff on Dec 9 2020, 11:02 AM.

Details

Summary

This CL changes the asm syntax for section flags, making them more like ELF
(previously "passive" was the only option). Now we also allow "G" to designate
COMDAT group sections. In these sections we set the appropriate comdat flag on
function symbols, and also avoid auto-creating a new section for them.

This also adds asm-based tests for the changes D92691 to go along with
the direct-to-object tests.

Differential Revision: https://reviews.llvm.org/D92952

Diff Detail

Event Timeline

dschuff created this revision.Dec 9 2020, 11:02 AM
dschuff published this revision for review.Dec 9 2020, 4:13 PM
dschuff retitled this revision from [WIP][WebAssembly] Support COMDAT sections in assembly to [WebAssembly] Support COMDAT sections in assembly syntax.
dschuff edited the summary of this revision. (Show Details)
dschuff added reviewers: sbc100, aardappel.

This is ready to review now.

Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2020, 4:13 PM
dschuff edited the summary of this revision. (Show Details)Dec 9 2020, 4:14 PM
dschuff updated this revision to Diff 310735.Dec 9 2020, 6:02 PM

Simplify WebAssemblyAsmParser, add test for its behavior, add TODO

dschuff updated this revision to Diff 310736.Dec 9 2020, 6:02 PM

fix rebase

OK, added a check for the behavior when the .section directive is missing for a function, and a TODO for the existing problem of honoring the user's specified section name.

sbc100 added a comment.Dec 9 2020, 7:59 PM

Great!

llvm/lib/MC/MCParser/WasmAsmParser.cpp
172

Combine into single condition?

llvm/lib/MC/WasmObjectWriter.cpp
1370

Did you mean to leave this in?

llvm/test/MC/WebAssembly/comdat-asm.ll
28 ↗(On Diff #310736)

I think we normally run with -asm-verbose=false to avoid these extra comments.

llvm/test/MC/WebAssembly/comdat-sections.s
6

I normally drop the .type foo,@function this its not needed.

24

Just one empty line here?

llvm/test/MC/WebAssembly/comdat.ll
5

Do we still need comdat-asm.ll given that this test runs in both modes?

aardappel accepted this revision.Dec 10 2020, 9:26 AM
This revision is now accepted and ready to land.Dec 10 2020, 9:26 AM
dschuff updated this revision to Diff 310946.Dec 10 2020, 9:51 AM
dschuff marked 3 inline comments as done.
  • review
dschuff updated this revision to Diff 310947.Dec 10 2020, 9:53 AM
  • Simplify; add test for forgetting to specify sectoin
  • review
  • remove comdat-asm.ll, rebase
sbc100 accepted this revision.Dec 10 2020, 2:04 PM

lgtm

( i didn't see any response the the two comments, one regarding the extra debugging output and the other regarding the include the .type foo,@function lines in test code).

oops, forgot to publish my comments.

llvm/lib/MC/WasmObjectWriter.cpp
1370

Yeah I figured it was useful to go along with the other debug prints behind LLVM_DEBUG in this file.

llvm/test/MC/WebAssembly/comdat-sections.s
6

Ah, you've actually hit on something interesting here. Currently the logic that sets the comdat flag on the function symbol is in WasmAsmParser::parseDirectiveType(), so it doesn't run if we leave out .type. That function also sets the symbol type. I don't see any logic anywhere else in WasmAsmParser or WebAssemblyAsmParser that sets the symbol type, but I guess it just defaults to function?

I tried it without the .type, and found that the tests all still worked! I found that that only the group flag on the section matters for creating the linking section. But other code still checks the comdat flag on the symbol, e.g. for creating imports. I updated the code for WebAssemblyAsmParser in this CL and I think it should work now, (i.e. it keeps the section group and symbol comdat proerties in sync). But I'm not sure how to test it. How would you specify an imported comdat function in assembly? Currently the only way to specify comdats is by using .section but that doesn't make sense for an import.

For now I put the .type there for foo, but left it off .bar, to exercise both codepaths. We can figure out the imported-comdat problem in a future CL.

llvm/test/MC/WebAssembly/comdat.ll
5

oh oops yes, I forgot I created comdat-asm.ll and then merged it in to here.

IIUC comdat is a propoerty shared by a group of a definitions (not declarations) so I don't see who an import could be part of a comdat group.

This revision was landed with ongoing or failed builds.Dec 10 2020, 2:46 PM
This revision was automatically updated to reflect the committed changes.