[PATCH] D55401: [WebAssembly] Fix assembler parsing of br_table.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 14 inline comments as done.
aardappel added inline comments.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:82
+  WebAssemblyOperand(KindTy K, SMLoc Start, SMLoc End)
+      : Kind(K), StartLoc(Start), EndLoc(End), BrL() {}
+
----------------
aheejin wrote:
> Do we need `BrL()`?
It's in a union, so how else would it initialize the vector?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:85
+  ~WebAssemblyOperand() {
+    if (isBrList()) BrL.~BrLOp();
+  }
----------------
aheejin wrote:
> Do we need this? `struct BrLOp` only contains a vector of integers.
It's in a union, so only should be destructed if it is off this type. What's a better way to do this?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:126
 
+  void addBrListOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && isBrList() && "Invalid BrList!");
----------------
aheejin wrote:
> Where is this method used? I also can't find where `addImmOperands` and `addRegOperands` are used either.
Methods in this file are called by the tablegen generated code in "WebAssemblyGenAsmMatcher.inc"


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:127
+  void addBrListOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && isBrList() && "Invalid BrList!");
+    for (auto Br : BrL.List) Inst.addOperand(MCOperand::createImm(Br));
----------------
aheejin wrote:
> What is `N` here? If it is always 1, do we need that argument?
This is called from generated code, I do not control that argument.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:146
+    case BrList:
+      OS << "BrList:" << BrL.List.size();
+      break;
----------------
aheejin wrote:
> Does only printing the size provide sufficient info? Can we print the contents too?
This print function is for showing operands when you are debugging the assembly matcher itself. It is not used for anything user facing, so does not need to be pretty.


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:253
+  O << "{";
+  for (unsigned i = OpNo, e = MI->getNumOperands(); i != e; ++i) {
+    if (i != OpNo)
----------------
aheejin wrote:
> Nit: [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | LLVM coding standard ]] says variable names should start with an upper case letter. I know, it's weird... :(
I usually get it right.. this piece of code was copied/adapted from the ARM code. There's a lot of non-conform code in LLVM it appears!


================
Comment at: test/MC/WebAssembly/basic-assembly.s:120
+# CHECK-NEXT:      br_table {0, 1, 2}  # 1: down to label4
+# CHECK-NEXT:                          # 2: down to label3
+# CHECK-NEXT:      end_block
----------------
aheejin wrote:
> - If we are gonna check these `down to labelN` part here, I guess it's better to add the same labels in the `end_block`s below.
> - Do we not print `down to labelN` comment for the default case?
I was wanting to not have these in the tests at all, but that would require a weird looking test for an empty like (which doesn't work), so I was forced to put these in.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55401