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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

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


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:146
+    case BrList:
+      OS << "BrList:" << BrL.List.size();
+      break;
----------------
aheejin wrote:
> 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?
Yes. Just trying to say this is very special purpose though, I don't think it is worth writing until you need it.


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:140
+              // This just needed to support -wasm-keep-registers
+              : !MI->getOperand(i).isImm())
         continue;
----------------
aheejin wrote:
> 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?
Added more explicit comments.


================
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 &&
----------------
aheejin wrote:
> Why was this deleted too?
Since we're basically not using variable_ops in the same way as before anymore (not annotated by TSFlags, not used in call in stack mode, and not used in br_table in the same way), keeping these checks didn't make much sense to me.


================
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 &&
----------------
aheejin wrote:
> Should this be deleted too?
See above.


================
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:
> 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.
Ok, added labels back in.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55401