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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

- Could you add "Fixes PR39384." to the CL/commit description?
- Please run clang-format (and maybe also clang-tidy) on the patch



================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:82
+  WebAssemblyOperand(KindTy K, SMLoc Start, SMLoc End)
+      : Kind(K), StartLoc(Start), EndLoc(End), BrL() {}
+
----------------
Do we need `BrL()`?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:85
+  ~WebAssemblyOperand() {
+    if (isBrList()) BrL.~BrLOp();
+  }
----------------
Do we need this? `struct BrLOp` only contains a vector of integers.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:126
 
+  void addBrListOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && isBrList() && "Invalid BrList!");
----------------
Where is this method used? I also can't find where `addImmOperands` and `addRegOperands` are used either.


================
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));
----------------
What is `N` here? If it is always 1, do we need that argument?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:128
+    assert(N == 1 && isBrList() && "Invalid BrList!");
+    for (auto Br : BrL.List) Inst.addOperand(MCOperand::createImm(Br));
+  }
----------------
clang-format


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:146
+    case BrList:
+      OS << "BrList:" << BrL.List.size();
+      break;
----------------
Does only printing the size provide sufficient info? Can we print the contents too?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:366
+        break;
+      }
       default:
----------------
Please clang-format this block


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:253
+  O << "{";
+  for (unsigned i = OpNo, e = MI->getNumOperands(); i != e; ++i) {
+    if (i != OpNo)
----------------
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... :(


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp:97
       if (i < Desc.getNumOperands()) {
-        assert(Desc.TSFlags == 0 &&
+        assert((Desc.TSFlags == 0 || IsBrTable) &&
                "WebAssembly non-variable_ops don't use TSFlags");
----------------
dschuff wrote:
> aardappel wrote:
> > dschuff wrote:
> > > aardappel wrote:
> > > > This is pretty yucky. I must I don't quite follow what these flags are used for, so not sure if we should remove these flags from br_table in the tablegen defs instead?
> > > I think we can remove the flags from br_table in tablegen because it doesn't use variable_ops anymore. I think they are only used for determining how the variable_ops should be printed.
> > variable_ops are still being used in call, but that doesn't print these either.
> > 
> > Want me to remove these flags as part of this CL or a next one?
> Might be a bit cleaner to be in this one but I don't have a strong opinion.
I think it should be fine to be removed within this CL, because we are switching from `variable_ops` to something else.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrControl.td:61
+def BR_TABLE_I32_S : NI<(outs), (ins brlist:$brl),
                         [], "true",
+                        "br_table \t$brl", 0x0e> {
----------------
Nit: Maybe these `[], "true"` can fit the line above?


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrControl.td:75
+def BR_TABLE_I64_S : NI<(outs), (ins brlist:$brl),
                         [], "true",
+                        "br_table \t$brl"> {
----------------
Here 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
----------------
- 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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55401