Page MenuHomePhabricator

WebAssembly: basic bitcode → assembly CodeGen test

Authored by jfb on Jul 20 2015, 1:46 PM.



Add a basic CodeGen bitcode test which (for now) only prints out the function name and nothing else. The current code merely implements the basic needed for the test run to not crash / assert. Getting to that point required:

  • Basic InstPrinter.
  • Basic AsmPrinter.
  • DiagnosticInfoUnsupported (not strictly required, but nice to have, duplicated from AMDGPU/BPF's ISelLowering).
  • Some SP and register setup in WebAssemblyTargetLowering.
  • Basic LowerFormalArguments.
  • GenInstrInfo.
  • Placeholder LowerFormalArguments.
  • Placeholder CanLowerReturn and LowerReturn.
  • Basic DAGToDAGISel::Select, which as well as GET_INSTRINFO_ENUM with
  • Remove WebAssemblyFrameLowering::determineCalleeSaves and rely on default.
  • Implement WebAssemblyFrameLowering::hasFP, same as AArch64's implementation.

Follow-up patches will implement a real AsmPrinter, which will require adding MI opcodes specific to WebAssembly.

Event Timeline

jfb updated this revision to Diff 30189.Jul 20 2015, 1:46 PM
jfb retitled this revision from to WebAssembly: basic bitcode → assembly CodeGen test.
jfb updated this object.
jfb added a reviewer: sunfish.
jfb added a subscriber: llvm-commits.
sunfish accepted this revision.Jul 21 2015, 2:05 PM
sunfish edited edge metadata.

lgtm, with comments

70 ↗(On Diff #30399)

"end anonymous namespace" seems more popular in LLVM.

80 ↗(On Diff #30399)

This line should probably be in a DEBUG macro.

40 ↗(On Diff #30399)

LLVM style now recommends against starting the comment with the function name now.

45 ↗(On Diff #30399)

Don't over-parenthesize return values.

21 ↗(On Diff #30399)

It doesn't seem common in LLVM to separate #includes with blank lines. Is there a reason for doing this?

102 ↗(On Diff #30399)

This should check the target and set *either* SP32 or SP64 as the stack pointer.

131 ↗(On Diff #30399)

It's actually likely that isn't appropriate. We don't have physical registers or stack arguments to deal with, which are the main things that file handles.

142 ↗(On Diff #30399)

We can assert this, since CanLowerReturn only returns true if there's at most one output value, right?

156 ↗(On Diff #30399)

It'd be nice to have a corresponding check in LowerReturn too.

158 ↗(On Diff #30399)

"yet". Even if we're planning to lower them in a different way, we should let people know that varargs are still planned to be supported :-).

160 ↗(On Diff #30399)

"yet" here too, because I don't ultimately see any reason we shouldn't support this.

This revision is now accepted and ready to land.Jul 21 2015, 2:05 PM
sunfish added inline comments.Jul 21 2015, 2:08 PM
37 ↗(On Diff #30399)

Should this be "wasm-asm-printer"? And should the one in InstPrinter/WebAssemblyInstPrinter.cpp be that too?

jfb updated this revision to Diff 30399.Jul 22 2015, 2:26 PM
jfb marked 11 inline comments as done.
jfb edited edge metadata.
  • Address sunfish's comments.
This revision was automatically updated to reflect the committed changes.
jfb added a comment.Jul 22 2015, 5:11 PM

I did a bit of grep-fu and found the following:

git grep "^#define DEBUG_TYPE" | cut -f3 -d' '| sort | uniq -c | sort -h | grep -v " 1 "
      2 "aarch64-disassembler"
      2 "arm-isel"
      2 "bugpoint"
      2 "codegen"
      2 "coverage-mapping"
      2 "delay-slot-filler"
      2 "flattencfg"
      2 "hexagon-shuffle"
      2 "indvars"
      2 "instsimplify"
      2 "jit"
      2 "mem2reg"
      2 "mips-lower"
      2 "mips-reg-info"
      2 "packets"
      2 "scheduler"
      2 "simplifycfg"
      2 "ssaupdater"
      2 "stackmaps"
      2 "subtarget-emitter"
      2 "x86-isel"
      3 "inline"
      3 "ir"
      3 "isel"
      3 "lli"
      3 "loop-unroll"
      3 "post-RA-sched"
      3 "region"
      4 "block-freq"
      4 "dwarfdebug"
      4 "misched"
      5 "legalize-types"
      5 "mips-isel"
      6 "pre-RA-sched"
      9 "mccodeemitter"
     11 "dyld"
     15 "instcombine"
     17 "regalloc"
     25 "asm-printer"

Is this worth fixing?

37 ↗(On Diff #30399)

That doesn't seem to be what other targets do:

git grep DEBUG_TYPE ./lib/Target/*/*AsmPrinter.cpp
lib/Target/AArch64/AArch64AsmPrinter.cpp:#define DEBUG_TYPE "asm-printer"
lib/Target/ARM/ARMAsmPrinter.cpp:#define DEBUG_TYPE "asm-printer"
lib/Target/BPF/BPFAsmPrinter.cpp:#define DEBUG_TYPE "asm-printer"
lib/Target/Hexagon/HexagonAsmPrinter.cpp:#define DEBUG_TYPE "asm-printer"
lib/Target/MSP430/MSP430AsmPrinter.cpp:#define DEBUG_TYPE "asm-printer"
lib/Target/Mips/MipsAsmPrinter.cpp:#define DEBUG_TYPE "mips-asm-printer"
lib/Target/PowerPC/PPCAsmPrinter.cpp:#define DEBUG_TYPE "asmprinter"
lib/Target/Sparc/SparcAsmPrinter.cpp:#define DEBUG_TYPE "asm-printer"
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:#define DEBUG_TYPE "asm-printer"
lib/Target/XCore/XCoreAsmPrinter.cpp:#define DEBUG_TYPE "asm-printer"

Should they? I can change all of them if so, probably in a different change.

jfb added a comment.Jul 22 2015, 5:14 PM

It actually seems to be on purpose:
So different targets enable their diagnostic with the same debug flag.

That's a good reason to leave it then. Thanks,