Differential D118545
[ARM] Fix 8-bit immediate overflow in the instruction of segmented stack prologue. Authored by ZhiyaoMa98 on Jan 29 2022, 4:04 PM.
Details It fixes the overflow of 8-bit immediate field in the emitted instruction that allocates large stacklet. For thumb2 targets, load large immediate by a pair of movw and movt instruction. For thumb1 and ARM targets, load large immediate by reading from literal pool.
Diff Detail
Event TimelineComment Actions Please add a test for this. llvm/test/CodeGen/ARM/segmented-stacks.ll already has some tests related to this, so it looks like you could add a couple of RUN lines for thumb and thumb2 targets, and add test_very_large that's like test_large but with an integer that's too large for ARM::MOVi.
Comment Actions For thumb1, the number of emitted instructions varies with the value of the 32-bit immediate. How should we set the cost for the new pseudo-instruction tMOVi32imm? For the existing t2MOVi32imm, the cost is set as IIC_iMOVix2. I understand it comes from one movw and one movt. But for variable cost, I am not sure what to do. I see the logic of expanding t2MOVi32imm in ARMExpandPseudo::ExpandMOV32BitImm, but I feel hard to understand them fully. Is there any tutorial that focuses on this part? Thank you for your review and comments, and help. Comment Actions Not sure about the discussion of MOVi32imm etc. In ARM mode, the existing MOVi32imm can't be used for arbitrary constants without armv6t2. For the general case, we emit a constant pool load. Similarly, on Thumb, we just never bothered to implement tMOVi32imm. Can we use emitThumbRegPlusImmediate/emitT2RegPlusImmediate/emitARMRegPlusImmediate here, instead of reimplementing it? Comment Actions I am looking into adding more tests, but existing test strikes me as strange. test_large() in llvm/test/CodeGen/Thumb/segmented-stacks.ll already tests for large stacklet allocation, but the test case is vain. The test case expects the following output: ...
push {r4, r5}
mov r5, sp
sub r5, #40192
...Even if without this patch, the output machine assembly code matches the expected output string and passes the test. But sub r5, #40192 is not a valid instruction on thumb. After passing through the assembler, the sub instruction becomes subs r5, #0: 00000078 <test_large>:
78: b430 push {r4, r5}
7a: 466d mov r5, sp
7c: 3d00 subs r5, #0
...How can I perform the test effectively? Comment Actions You'll want to have the RUN lines for the thumb tests to have a --check-prefix argument to FileCheck that's unique, and use that to check for the thumb-specific codegen, e.g. ; RUN: llc < %s -mtriple=arm-linux-androideabi -mattr=+v4t -verify-machineinstrs | FileCheck %s -check-prefix=ARM-android ; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -mattr=+v4t -verify-machineinstrs | FileCheck %s -check-prefix=ARM-linux ; RUN: llc < %s -mtriple=thumb-linux-androideabi -mattr=+v4t -verify-machineinstrs | FileCheck %s -check-prefix=THUMB-android ; RUN: llc < %s -mtriple=thumb-linux-unknown-gnueabi -mattr=+v4t -verify-machineinstrs | FileCheck %s -check-prefix=THUMB-linux then later ; THUMB-android: test_large:
; THUMB-android: push {r4, r5}
; THUMB-android-NEXT: whatever the expected sp-setting instruction sequence isComment Actions Let me clarify the problem. The test compares the output at the machine assembly level, i.e. ARM (thumb) assembly instruction. However, some syntactically correct ARM (thumb) assembly does not generate binary instruction with the same semantic. For example, sub r5, #40192 is a syntactically correct ARM (thumb) assembly, but #40192 does not fit into 8-bit immediate field. Surprisingly the assembler accepts the assembly instruction and emits the binary instruction subs r5, #0, without reporting an error. The following command tests at the machine assembly level. It sees sub r5, #40192 and considers it correct. The following command generates binary object. It should trigger an error when it tries to assemble sub r5, #40192 in thumb1, but does not. So my point is the current test code has already included the large stack frame test case, but the bug is not detected and we are unable to detect it in this way. We should rewrite the test, but I do not know how the test can be written to reveal the bug I am trying to resolve. Comment Actions When the integrated assembler is used directly by the compiler, as opposed to parsing textual assembler, it uses a different codepath which passes the operands directly, which skips converting them to/from text. This codepath ends up skipping some kinds of operand validation; we don't expect to be generating invalid instructions. We do have a method ARMBaseInstrInfo::verifyInstruction which is used by the machine verifier to validate target-specific properties of instructions. It currently only verifies immediates for load/store instructions, not arithmetic, so it doesn't currently catch this. Improvements are definitely welcome, though. Comment Actions To clarify, if you try to assemble a file containing the line sub r5, #40192 on a Thumb target, it will print an error. The issue you're seeing with an impossible instruction only shows up for instructions generated internally by the compiler itself. ARMBaseInstrInfo::verifyInstruction can be tested using a MachineIR (MIR) test. See llvm/test/CodeGen/ARM/machine-verifier.mir . Comment Actions The pseudo-instruction ARM::MOVi32imm seems to be broken. c0: e3e04fff mvn r4, #1020 ; 0x3fc c4: e24444ff sub r4, r4, #-16777216 ; 0xff000000 No existing code is using ARM::MOVi32imm. Is the pseudo-instruction really broken or I misunderstood something? Comment Actions When we don't have movw/movt, there is no two-instruction sequence to materialize an arbitrary immediate, so we only use ARM::MOVi32imm in very limited cases. See https://github.com/llvm/llvm-project/blob/807e2f12fab52c6abf3e89c02eec0f585b3b8f22/llvm/lib/Target/ARM/ARMInstrInfo.td#L836 and https://github.com/llvm/llvm-project/blob/807e2f12fab52c6abf3e89c02eec0f585b3b8f22/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp#L970 . (Maybe ARMExpandPseudo::ExpandMOV32BitImm should have an assertion to make this clear.) Comment Actions
Comment Actions LGTM @john.brawn Any further comment? @ZhiyaoMa98 I assume you don't have commit access; how do you want to be credited in the "author" line of the git commit? Comment Actions @efriedma | ||||||||||||
We already have pseudo-instructions for mov of 32-bit immediate in arm and thumb2, so instead of having a function to emit the code sequence here it would be better I think to create a tMOVi32imm pseudo-instruction and generate the instruction sequence when expanding it.