Uses the named operands tablegen feature to look up the indices of
offset, address, and p2align operands for all load and store
instructions. This replaces brittle, incorrect logic for identifying
loads and store when eliminating frame indices, which previously
crashed on bulk-memory ops. It also cleans up the SetP2Alignment pass.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 28824 Build 28823: arc lint + arc unit
Event Timeline
Nice! I'm learning more tablegen stuff everytime I see your CL 👍I have a few questions:
llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp | ||
---|---|---|
74 | Nit: how about making it a variable as you did for OffsetOperandNum below, like unsigned AddrOperandNum = WebAssembly::getNamedOperandIdx( MI.getOpcode(), WebAssembly::OpName::addr)); if (AddrOperandNum == FIOperandNum) { ... | |
86 | While this is a lot more general and better than the previous code and the fix looks correct, I can't find any bulk memory instructions in its td file that has addr operands. I don't think the bulk memory test cases enter this if block. Is that intended? | |
llvm/lib/Target/WebAssembly/WebAssemblySetP2AlignOperands.cpp | ||
94 | Why not changed? I see the previous code didn't even touch Changed, but aren't we supposed set it to true when if (P2AlignOpNum != -1) holds? | |
llvm/test/CodeGen/WebAssembly/bulk-memory.ll | ||
147 |
|
llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp | ||
---|---|---|
74 | Good idea, will do. | |
86 | Very much intended! The previous errors were due to bulk memory operations were entering this block, which tried to extract Offset operands that did not exist. Now they do not enter this block, which fixes the error. | |
llvm/lib/Target/WebAssembly/WebAssemblySetP2AlignOperands.cpp | ||
94 | I had this question too, but I did not know the full context of what this return value means and how it is used, so I wanted to be conservative and not change the behavior of the code. It looks like setting Changed to true if we call rewriteP2Align doesn't break anything, so we can go with that. | |
llvm/test/CodeGen/WebAssembly/bulk-memory.ll | ||
147 | Updated to reflect my findings from investigating TransientStackAlignment. |
LGTM w/ a comment question.
llvm/test/CodeGen/WebAssembly/bulk-memory.ll | ||
---|---|---|
147 | The comment still says "stack_pointer is bumped". What I meant was __stack_pointer is NOT bumped, i.e., there's no global.set __stack_pointer in the generated code. Isn't __stack_pointer here just used to compute the frame index? |
llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp | ||
---|---|---|
30 | do you need to #undef GET_INSTRINFO_CTOR_DTOR before you include the .inc file again? |
llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp | ||
---|---|---|
30 | No, the undef is taken care of inside the included file. This is a common pattern for including all TableGen files. | |
llvm/test/CodeGen/WebAssembly/bulk-memory.ll | ||
147 | There are two "stack pointers" in play here. The stack pointer that gets bumped is LLVM's idea of a stack pointer, which is subject to StackAlignment and TransientStackAlignment and is used as the base address for the offsets of locals. The global __stack_pointer that participates in the WebAssembly ABI does not get bumped because we're using the red zone, but that is an invention of the WebAssembly backend that the rest of LLVM is not aware of. I guess I can make this less confusing by saying "The stack pointer is bumped" instead of "__stack_pointer is bumped". Does that seem reasonable to you? |
llvm/test/CodeGen/WebAssembly/bulk-memory.ll | ||
---|---|---|
147 | Yeah that sounds clearer. |
do you need to #undef GET_INSTRINFO_CTOR_DTOR before you include the .inc file again?
edit: also above.