This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix assembler parsing of br_table.
ClosedPublic

Authored by aardappel on Dec 6 2018, 4:50 PM.

Details

Summary

We use variable_ops in the tablegen defs to denote the list of
branch targets in br_table, but unlike other uses of variable_ops
(e.g. call) the these branch targets need to actually be encoded in the
instruction. The existing tables for variable_ops cause not operands
to be accepted by the assembly matcher.

Following the example of ARM:
https://github.com/llvm-mirror/llvm/blob/2cc0a7da876c1d8c32775b0119e1e15aaa759b9e/lib/Target/ARM/ARMInstrInfo.td#L550-L555
we introduce a new operand type to capture this list, and we use the
same {} syntax as ARM as well to differentiate them from regular
integer operands.

Diff Detail

Repository
rL LLVM

Event Timeline

aardappel created this revision.Dec 6 2018, 4:50 PM

Hey reviewers, consider this WiP since I don't know if this is the most elegant way to do it. Your feedback very welcome!

aardappel marked an inline comment as done.Dec 6 2018, 4:53 PM
aardappel added inline comments.
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
97 ↗(On Diff #177086)

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?

dschuff added inline comments.Dec 7 2018, 1:44 PM
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
97 ↗(On Diff #177086)

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.

aardappel marked an inline comment as done.Dec 10 2018, 10:31 AM
aardappel added inline comments.
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
97 ↗(On Diff #177086)

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?

dschuff added inline comments.Dec 10 2018, 2:29 PM
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
97 ↗(On Diff #177086)

Might be a bit cleaner to be in this one but I don't have a strong opinion.

  • Could you add "Fixes PR39384." to the CL/commit description?
  • Please run clang-format (and maybe also clang-tidy) on the patch
lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
82 ↗(On Diff #177086)

Do we need BrL()?

85 ↗(On Diff #177086)

Do we need this? struct BrLOp only contains a vector of integers.

126 ↗(On Diff #177086)

Where is this method used? I also can't find where addImmOperands and addRegOperands are used either.

127 ↗(On Diff #177086)

What is N here? If it is always 1, do we need that argument?

128 ↗(On Diff #177086)

clang-format

146 ↗(On Diff #177086)

Does only printing the size provide sufficient info? Can we print the contents too?

366 ↗(On Diff #177086)

Please clang-format this block

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
253 ↗(On Diff #177086)

Nit: LLVM coding standard says variable names should start with an upper case letter. I know, it's weird... :(

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
97 ↗(On Diff #177086)

I think it should be fine to be removed within this CL, because we are switching from variable_ops to something else.

lib/Target/WebAssembly/WebAssemblyInstrControl.td
61 ↗(On Diff #177086)

Nit: Maybe these [], "true" can fit the line above?

75 ↗(On Diff #177086)

Here too

test/MC/WebAssembly/basic-assembly.s
120 ↗(On Diff #177086)
  • If we are gonna check these down to labelN part here, I guess it's better to add the same labels in the end_blocks below.
  • Do we not print down to labelN comment for the default case?

Oh, and just FYI: the ARM code Github link in your CL description wouldn't work once they make changes to the code. You can take a permalink by clicking three dots in the left side of the code and click "Copy permalink", which will get you a link of the specified line in the latest commit. It looks like this: https://github.com/llvm-mirror/llvm/blob/2cc0a7da876c1d8c32775b0119e1e15aaa759b9e/lib/Target/ARM/ARMInstrInfo.td#L550
Also you can do that for for a block of code to by selecting a block by clicking shift and create a permalink for that, like
https://github.com/llvm-mirror/llvm/blob/2cc0a7da876c1d8c32775b0119e1e15aaa759b9e/lib/Target/ARM/ARMInstrInfo.td#L550-L555

aardappel marked 14 inline comments as done.Dec 10 2018, 5:13 PM
aardappel added inline comments.
lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
82 ↗(On Diff #177086)

It's in a union, so how else would it initialize the vector?

85 ↗(On Diff #177086)

It's in a union, so only should be destructed if it is off this type. What's a better way to do this?

126 ↗(On Diff #177086)

Methods in this file are called by the tablegen generated code in "WebAssemblyGenAsmMatcher.inc"

127 ↗(On Diff #177086)

This is called from generated code, I do not control that argument.

146 ↗(On Diff #177086)

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.

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
253 ↗(On Diff #177086)

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!

test/MC/WebAssembly/basic-assembly.s
120 ↗(On Diff #177086)

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.

aardappel updated this revision to Diff 177633.Dec 10 2018, 5:16 PM

Code review fixes.

aardappel edited the summary of this revision. (Show Details)Dec 10 2018, 5:17 PM

ARM link updated as permalink, thanks.

aheejin added inline comments.Dec 10 2018, 6:32 PM
lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
82 ↗(On Diff #177086)

Oh right. Sorry I missed this was a union.

85 ↗(On Diff #177086)

Sorry, nevermind. :)

146 ↗(On Diff #177086)

But wouldn't you want to know what's inside when you're debugging too?

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
139 ↗(On Diff #177633)

Missing 'is' after 'this'?

140 ↗(On Diff #177633)

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 ifs with continues with each of them, maybe with one-line comment for each as well?

227 ↗(On Diff #177633)

Why was this deleted too?

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
135 ↗(On Diff #177633)

Should this be deleted too?

test/MC/WebAssembly/basic-assembly.s
120 ↗(On Diff #177086)

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.

aardappel marked 17 inline comments as done.Dec 13 2018, 2:11 PM
aardappel added inline comments.
lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
146 ↗(On Diff #177086)

Yes. Just trying to say this is very special purpose though, I don't think it is worth writing until you need it.

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
140 ↗(On Diff #177633)

Added more explicit comments.

227 ↗(On Diff #177633)

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.

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
135 ↗(On Diff #177633)

See above.

test/MC/WebAssembly/basic-assembly.s
120 ↗(On Diff #177086)

Ok, added labels back in.

aardappel updated this revision to Diff 178135.Dec 13 2018, 2:13 PM
aardappel marked 3 inline comments as done.

More code review fixes.

aheejin added inline comments.Dec 14 2018, 7:09 AM
lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
227 ↗(On Diff #177633)

Oh what I asked is about

assert(OpNo < Desc.getNumOperands() &&
       "Unexpected floating-point immediate as a non-fixed operand");

Is this related to TSFlags or variable_ops too?

143 ↗(On Diff #178135)

variable ops_operand -> variable_ops operand ?

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
135 ↗(On Diff #177633)

Oh what I asked is about

assert(OpNo < Desc.getNumOperands() &&
       "Unexpected floating-point immediate as a non-fixed operand");

Is this related to TSFlags or variable_ops too?

aardappel marked 2 inline comments as done.Dec 17 2018, 12:30 PM
aardappel added inline comments.
lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
227 ↗(On Diff #177633)

Yes, that is what I was claiming. We're not using TSFlags at all anymore.

Fixed spelling error.

aheejin accepted this revision.EditedDec 17 2018, 1:06 PM

Probably I don't have the context of what that floating point related assertion means and how it is related to TSFlags. Just curious, could you explain? Otherwise LGTM!

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
227 ↗(On Diff #177633)

Sorry, but I don't understand, maybe I don't have the context of this assertion. How is

assert(OpNo < Desc.getNumOperands() &&
       "Unexpected floating-point immediate as a non-fixed operand");

related to TSFlags?

This revision is now accepted and ready to land.Dec 17 2018, 1:06 PM

@aheejin they were protecting against a floating point operand showing up as part of variable_ops. I don't think think this was a particularly crucial assert to start with.

This revision was automatically updated to reflect the committed changes.