This code is based on AArch64 for modern backend good practice, and NVPTX for
virtual ISA concerns.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Cool. Here's a first round of comments.
lib/Target/WebAssembly/CMakeLists.txt | ||
---|---|---|
3 ↗ | (On Diff #29358) | Please keep these rules alphabetized. |
lib/Target/WebAssembly/Makefile | ||
16 ↗ | (On Diff #29358) | Common practice in these Makefiles is to indent subsequent lines to be to the right of the '='. |
lib/Target/WebAssembly/WebAssemblyInstrAtomics.td | ||
12 ↗ | (On Diff #29358) | For full doxygen happiness, these should get triple-slash comments, as is done in other files. |
lib/Target/WebAssembly/WebAssemblyInstrCall.td | ||
1 ↗ | (On Diff #29358) | If we have extra characters available in the top-line comment, it's nice to leave in some of the '='s that LLVM typically uses at the beginning and end. |
lib/Target/WebAssembly/WebAssemblyInstrConv.td | ||
1 ↗ | (On Diff #29358) | "Conv" in closely-related contexts is short for Convention, as in CallingConv. I'm ok with the filename here, but can you make the top-level comment clarify that this is about conversions? |
lib/Target/WebAssembly/WebAssemblyInstrInfo.td | ||
40 ↗ | (On Diff #29358) | Is there a reason for reordering these? Operands and DAG Nodes and DAG Node Types are all inter-related so it's nice to have them together. |
50 ↗ | (On Diff #29358) | If comma and conditional end up being included in WebAssembly, they'll have embedded control flow, so we won't be able to represent them explicitly in either SelectionDAG or MachineInstrs. |
lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp | ||
49 ↗ | (On Diff #29358) | This should also set FP32/FP64 as reserved, at least when TFI->hasFP(), but maybe always, since these are virtual registers anyway. |
56 ↗ | (On Diff #29358) | report_fatal_error is for legitimate conditions that codegen can't handle, not for reporting bugs in codegen. llvm_unreachable is more appropriate here because it's just a "bug" that this function isn't implemented yet. |
61 ↗ | (On Diff #29358) | Using unsigned instead of unsigned char for holding register values. |
65 ↗ | (On Diff #29358) | We should static_cast this to WebAssemblyFrameLowering so that the hasFP call can be easily devirtualized. |
lib/Target/WebAssembly/WebAssemblyRegisterInfo.td | ||
13 ↗ | (On Diff #29358) | More triple-slash comments. |
30 ↗ | (On Diff #29358) | WebAssembly won't support it right away, and it's not yet decided whether it will support it in the future. But actually, we need separate registers anyway so that we can put them in separate register classes. |
43 ↗ | (On Diff #29358) | This code comes from NVPTXRegisterInfo.td, and we are doing something similar here, but I'm curious what it's really needed for. Could we try leaving it out and seeing what breaks? |
Updated, PTAL.
lib/Target/WebAssembly/Makefile | ||
---|---|---|
16 ↗ | (On Diff #29404) | Those are tabs (same as for other Makefiles), they just don't appear nicely in phab. |
lib/Target/WebAssembly/WebAssemblyInstrInfo.td | ||
50 ↗ | (On Diff #29404) | Good point, I removed them. |
lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp | ||
49 ↗ | (On Diff #29404) | Oops, I added FP later and forgot to add it here too. Changed to a range-based for loop too. |
61 ↗ | (On Diff #29404) | Imagine all the bits we're wasting! ;-) |
65 ↗ | (On Diff #29404) | I'll send a separate PR to do the same in other backends. |
lib/Target/WebAssembly/WebAssemblyRegisterInfo.td | ||
30 ↗ | (On Diff #29404) | Are you agreeing, or suggesting something? |
43 ↗ | (On Diff #29404) | I'll add a todo instead to try it out later, if needed? I'd rather follow what others have done, and then try to change it, seems like we'll have less WTF moments that way. |
Looks good.
lib/Target/WebAssembly/WebAssemblyRegisterInfo.td | ||
---|---|---|
30 ↗ | (On Diff #29409) | I think I'm just suggesting the comment say something like "may someday support" instead of "supports". |
- Address sunfish's comments.
- Use the shiny new devirtualized getFrameLowering from D11093.
- 'May someday support'.