This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Create separation between MC and CodeGen layers
ClosedPublic

Authored by rnk on Jul 27 2023, 11:33 AM.

Details

Summary

Move WebAssemblyUtilities from Utils to the CodeGen library. It
primarily deals in MIR layer types, so it really lives in the CodeGen
library.

Move a variety of other things around to try create better separation.

See issue #64166 for more info on layering.

I wasn't sure what to do with llvm/include/CodeGen/WasmAddressSpaces.h.
It has no other users, so it seems like it should live in lib/Target,
but it was just recently moved to CodeGen. I worked around the issue in
the Bazel build by adding it to the CodeGenTypes library, but this seems
awkard. Maybe it should live in BinaryFormat or IR.

Diff Detail

Event Timeline

rnk created this revision.Jul 27 2023, 11:33 AM
rnk requested review of this revision.Jul 27 2023, 11:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 11:33 AM
Herald added a subscriber: aheejin. · View Herald Transcript
dschuff edited reviewers, added: aheejin; removed: dschuff.Jul 29 2023, 8:34 PM

I'm out of office, I think @aheejin can review (also /cc @sunfish )

Thanks! This is much clearer. About WasmAddressSpaces.h, I have no idea why it was put into that common lib/CodeGen directory; I haven't reviewed D139010 myself. But it looks it is currently only used in lib/Target/WebAssembly/Utils/WebAssemblyTypeUtilities.h, so I guess it makes sense to move it to lib/Target/WebAssembly/Utils/ too? If we move it to lib/Target/WebAssembly/, we again end up creating dependency on WebAssemblyCodeGen library, which I think is what you are trying to remove. cc @pmatos in case he has other suggestions.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
428–439

I think this can stay here if we change the argument from MachineInstr to unsigned Opc, which is already the case for all other methods in this file. I'm not sure why this function was an exception.

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
20–21

Can you run clang-format? It looks the order of includes needs to change here and several other places.

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.h
35–45 ↗(On Diff #544871)

We can put this back to where it was with the argument changed: See the comment in WebAssemblyMCTargetDesc.h.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
167–171

I think we can just delete this and make the callsites use WebAssembly::regClassToValType directly.

rnk updated this revision to Diff 551300.Aug 17 2023, 3:31 PM
rnk marked 4 inline comments as done.
  • apply fixes
rnk added a comment.Aug 17 2023, 3:31 PM

Thanks for the review, hopefully all comments are addressed.

aheejin accepted this revision.Aug 18 2023, 2:01 PM

Thanks!

This revision is now accepted and ready to land.Aug 18 2023, 2:01 PM
This revision was landed with ongoing or failed builds.Aug 18 2023, 2:13 PM
This revision was automatically updated to reflect the committed changes.