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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin 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() {}
+
----------------
aardappel wrote:
> aheejin wrote:
> > Do we need `BrL()`?
> It's in a union, so how else would it initialize the vector?
Oh right. Sorry I missed this was a union.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:85
+  ~WebAssemblyOperand() {
+    if (isBrList()) BrL.~BrLOp();
+  }
----------------
aardappel wrote:
> 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?
Sorry, nevermind. :)


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:146
+    case BrList:
+      OS << "BrList:" << BrL.List.size();
+      break;
----------------
aardappel wrote:
> 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.
But wouldn't you want to know what's inside when you're debugging too?


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:139
+              ? Desc.OpInfo[i].OperandType != WebAssembly::OPERAND_BASIC_BLOCK
+              // This just needed to support -wasm-keep-registers
+              : !MI->getOperand(i).isImm())
----------------
Missing 'is' after 'this'?


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:140
+              // This just needed to support -wasm-keep-registers
+              : !MI->getOperand(i).isImm())
         continue;
----------------
It's not related to changes in this CL, but this `if` statement is a bit hard to understand. Could you possibly break it down to two `if`s with `continue`s with each of them, maybe with one-line comment for each as well?


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:227
-    assert(OpNo < Desc.getNumOperands() &&
-           "Unexpected floating-point immediate as a non-fixed operand");
-    assert(Desc.TSFlags == 0 &&
----------------
Why was this deleted too?


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp:135
-      assert(i < Desc.getNumOperands() &&
-             "Unexpected floating-point immediate as a non-fixed operand");
-      assert(Desc.TSFlags == 0 &&
----------------
Should this be deleted too?


================
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
----------------
aardappel wrote:
> 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.
Yeah but we are checking branch label comments in other instructions within this test too, so it wouldn't hurt to denote where `label3` and `label4` is anyway, which is also good for readability.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55401