[PATCH] D156472: [WebAssembly] Create separation between MC and CodeGen layers

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

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 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h:428-439
-inline bool isBrTable(const MachineInstr &MI) {
-  switch (MI.getOpcode()) {
-  case WebAssembly::BR_TABLE_I32:
-  case WebAssembly::BR_TABLE_I32_S:
-  case WebAssembly::BR_TABLE_I64:
-  case WebAssembly::BR_TABLE_I64_S:
-    return true;
----------------
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.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:21
 #include "Utils/WebAssemblyTypeUtilities.h"
-#include "Utils/WebAssemblyUtilities.h"
+#include "WebAssemblyUtilities.h"
 #include "WebAssembly.h"
----------------
Can you run clang-format? It looks the order of `include`s needs to change here and several other places.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.h:35-45
+inline bool isBrTable(const MachineInstr &MI) {
+  switch (MI.getOpcode()) {
+  case WebAssembly::BR_TABLE_I32:
+  case WebAssembly::BR_TABLE_I32_S:
+  case WebAssembly::BR_TABLE_I64:
+  case WebAssembly::BR_TABLE_I64_S:
+    return true;
----------------
We can put this back to where it was with the argument changed: See the comment in `WebAssemblyMCTargetDesc.h`.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:167-171
+static wasm::ValType regClassToValType(const TargetRegisterClass *RC) {
+  assert(RC != nullptr);
+  return WebAssembly::regClassToValType(RC->getID());
+}
+
----------------
I think we can just delete this and make the callsites use [[ https://github.com/llvm/llvm-project/blob/3b2f3238a4d7d8935188f1264df0887dead1e9ea/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTypeUtilities.h#L62-L63 | WebAssembly::regClassToValType ]] directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156472