Only support patching FunctionEntry/FunctionExit/FunctionTailExit for now.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
D140725 is abandoned. Let me defer this change until we support 64bit PC-relative relocation for SymA - SymB.
Update: R_LARCH_64_PCREL can be emitted after D153872. And I'd like to ask @Ami-zhang to update this patch later.
llvm/test/CodeGen/LoongArch/xray-attribute-instrumentation.ll | ||
---|---|---|
12 | CHECK-COUNT-11 |
llvm/test/CodeGen/LoongArch/xray-attribute-instrumentation.ll | ||
---|---|---|
2 | I think you want to read my recent updates to other xray-attribute-instrumentation.ll. Certain changes can make the test less sensitive to non-xray updates. |
compiler-rt/lib/xray/xray_loongarch64.cpp | ||
---|---|---|
154 | I know you copy the pattern from other patterns, but we should use TODO here. An unimplemented minor feature is not FIXME. | |
compiler-rt/lib/xray/xray_trampoline_loongarch64.S | ||
28 | You can use .irpc to simplify this a bit. See libunwind/src/UnwindRegistersSave.S | |
compiler-rt/test/xray/TestCases/Posix/arg1-arg0-logging.cpp | ||
9 ↗ | (On Diff #537564) | I added aarch64 support. You may rebase and add loongarch64 on the REQUIRES: line. |
compiler-rt/test/xray/TestCases/Posix/basic-filtering.cpp | ||
26 ↗ | (On Diff #537564) | Most REQUIRES: x86_64-target-arch are unnecessarily constrained. I just removed them. You may rebase and avoid changes to these test files. |
compiler-rt/lib/xray/xray_loongarch64.cpp | ||
---|---|---|
24 | I think the PO_ style actually harms readability. Most instructions are used just once. It's more readable just using the magic number when it is needed paired with an inline comment Address[10] = insn2RI12(0x02c00000, RegNum::RN_S, ...) // addi.d ... |
Since R_LARCH_64_PCREL has been supported, we can use version 2 xray and the patch summary should be modified accordingly.
llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp | ||
---|---|---|
187 | should use version 2: recordSled(BeginOfSled, MI, Kind, 2); |
compiler-rt/lib/xray/xray_loongarch64.cpp | ||
---|---|---|
24 | Ok, updated it. Thanks for your suggestion. | |
154 | Done. | |
compiler-rt/lib/xray/xray_trampoline_loongarch64.S | ||
28 | Done. | |
compiler-rt/test/xray/TestCases/Posix/arg1-arg0-logging.cpp | ||
9 ↗ | (On Diff #537564) | Rebased. |
compiler-rt/test/xray/TestCases/Posix/basic-filtering.cpp | ||
26 ↗ | (On Diff #537564) | Rebased. |
llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp | ||
187 | Yes, updated it. | |
llvm/test/CodeGen/LoongArch/xray-attribute-instrumentation.ll | ||
12 | Done. |
compiler-rt/lib/xray/xray_loongarch64.cpp | ||
---|---|---|
23–26 | I think people usually just declare the register ABI names with decimal numbers for readability? This is minor but I personally find it slightly distracting to have to think about what's 0xc in decimal (that's the case even though I'm already very familiar with these kinds of thing). Not sure if others also prefer decimals though... | |
33 | Parens not needed in this case. (If you want to enhance readability for those people having difficulties remembering precedence and associativeness of operators, you should instead put the parens around Rj << 5 and Imm << 10...) I'd suggest re-ordering the operands too, to imitate the expected bit layout of the result. | |
40 | Similarly here. | |
43–48 | Does this look *very* similar to encodeInstruction2RI12? ;-) Actually, if you don't bounds-check the operands, you only need one helper for each combination of slots involved. Check for example the static uint32_t insn implementation in my D138135, or the auto-generated encoding helpers for the QEMU TCG LoongArch64 port, for some alternative approaches. | |
59 | .tmpN: for it to not look like a directive? | |
72 | All-lowercase i.e. tracing for consistency? Or am I missing something? | |
74 | I think idiomatically it's not "create/delete stack frame" but rather "(de-)allocate" or "setup/teardown"... anyway please fix the usage of articles (put the "the"'s properly) and also add a space after the ; because it's usually aesthetically better. This is all minor though, and it's not like people will misunderstand anything otherwise, but rather it's mostly just me pushing for more natural English usage. | |
81 | ("set back" happens to be an idiomatic phrase verb so I got confused here for ~1 second, so I think it's probably easier to just avoid this coincidence.) | |
compiler-rt/lib/xray/xray_trampoline_loongarch64.S | ||
84 | Use pseudo-instruction for idiomatic expression of intent. | |
llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp | ||
152–177 | We don't have to repeat the instruction pattern when we can just refer people to the source file containing this. Also the comment seems to be explanation for the magic number 11, so it should probably come before the definition. |
compiler-rt/lib/xray/xray_loongarch64.cpp | ||
---|---|---|
23–26 | Ok, use decimal numbers to declare the register ABI names. | |
33 | What you said makes sense. Thanks for your suggestion and attentiveness. I have adjusted parens and operands order. | |
40 | Done. | |
43–48 | It is indeed similar to encodeInstruction2RI12. Given its exclusive use in this context, I prefer the unified 2RIx format. Thanks for the suggestion. While we appreciate it, we are more inclined to use one 2RIx format helper here. | |
59 | Use the real .Lxray_sled_endN. | |
72 | It should be a copy from mips. I think it can be written tracing hook or TracingHook. To maintain all-lowercase consistency here, I use tracing hook. | |
74 | Ok, a space after the ; is added. About article the, i also added it. But i am not sure whether the usage of the is proper. If there are any further issues , I would greatly appreciate your feedback. Thanks for your effort. | |
81 | Done. | |
compiler-rt/lib/xray/xray_trampoline_loongarch64.S | ||
84 | Done. | |
llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp | ||
152–177 | Done. |
Thanks for following up with the suggestions. This now looks mostly okay to me; let's wait for more people to chip in!
Ah, the patch summary probably needs some update as well. We no longer care about version 0 and the backend changes for R_LARCH_64_PCREL are already in, for example.
compiler-rt/lib/xray/xray_trampoline_loongarch64.S | ||
---|---|---|
21 | The symbol needs to be hidden and consider using some macros. See xray_trampoline_AArch64.S |
compiler-rt/lib/xray/xray_loongarch64.cpp | ||
---|---|---|
31 | Early xray code unfortunately does not respect the conventional style that well. |
One nit about the test.
llvm/test/CodeGen/LoongArch/xray-attribute-instrumentation.ll | ||
---|---|---|
25 | See llvm/test/CodeGen/X86/xray-attribute-instrumentation.ll. I use something like [[TMP:.Ltmp[0-9]+]]: so that we don't need to adjust the test if the compiler happens to produce more .Ltmp* symbols. |
llvm/test/CodeGen/LoongArch/xray-attribute-instrumentation.ll | ||
---|---|---|
25 | Done. |
Make xray-attribute-instrumentation.ll less sensitive to .Ltmp/.Lxray_fn_idx label changes
I think the PO_ style actually harms readability. Most instructions are used just once. It's more readable just using the magic number when it is needed paired with an inline comment
Address[10] = insn2RI12(0x02c00000, RegNum::RN_S, ...) // addi.d ...