This is an archive of the discontinued LLVM Phabricator instance.

[XRay] Add initial support for loongarch64
ClosedPublic

Authored by SixWeining on Dec 28 2022, 6:08 AM.

Details

Summary

Only support patching FunctionEntry/FunctionExit/FunctionTailExit for now.

Diff Detail

Event Timeline

SixWeining created this revision.Dec 28 2022, 6:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 28 2022, 6:08 AM
SixWeining requested review of this revision.Dec 28 2022, 6:08 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptDec 28 2022, 6:08 AM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits. · View Herald Transcript
SixWeining planned changes to this revision.Dec 28 2022, 5:38 PM

D140725 is abandoned. Let me defer this change until we support 64bit PC-relative relocation for SymA - SymB.

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

MaskRay added inline comments.
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.

Ami-zhang updated this revision to Diff 537564.Jul 5 2023, 7:31 PM

(1) Rebase
(2) Make some minor changes to xray-attribute-instrumentation.ll

MaskRay added inline comments.Jul 5 2023, 9:35 PM
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.

MaskRay added inline comments.Jul 5 2023, 9:37 PM
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);
Ami-zhang added inline comments.Jul 7 2023, 1:37 AM
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.

Ami-zhang updated this revision to Diff 538022.Jul 7 2023, 1:40 AM

Rebase and make some improvements.

xen0n added inline comments.Jul 7 2023, 2:11 AM
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.

Ami-zhang added inline comments.Jul 11 2023, 12:59 AM
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.

xen0n accepted this revision.Jul 11 2023, 1:19 AM

Thanks for following up with the suggestions. This now looks mostly okay to me; let's wait for more people to chip in!

This revision is now accepted and ready to land.Jul 11 2023, 1:19 AM
xen0n added a comment.Jul 11 2023, 1:20 AM

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.

Ami-zhang edited the summary of this revision. (Show Details)Jul 11 2023, 1:33 AM
MaskRay requested changes to this revision.Jul 11 2023, 9:54 AM
MaskRay added inline comments.
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

This revision now requires changes to proceed.Jul 11 2023, 9:54 AM
MaskRay added inline comments.Jul 11 2023, 9:56 AM
compiler-rt/lib/xray/xray_loongarch64.cpp
31

Early xray code unfortunately does not respect the conventional style that well.
The usual order is static inline

Ami-zhang added inline comments.Jul 12 2023, 3:12 AM
compiler-rt/lib/xray/xray_loongarch64.cpp
31

Done, adjusted to static inline.

compiler-rt/lib/xray/xray_trampoline_loongarch64.S
21

Ok, I have modified them.
Thanks for your suggestion.

Ami-zhang updated this revision to Diff 539465.Jul 12 2023, 3:14 AM

Hide some symbols and using some macros in asm.

MaskRay accepted this revision.Jul 12 2023, 6:27 PM

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.

This revision is now accepted and ready to land.Jul 12 2023, 6:27 PM
Ami-zhang added inline comments.Jul 12 2023, 7:02 PM
llvm/test/CodeGen/LoongArch/xray-attribute-instrumentation.ll
25

Done.

Ami-zhang updated this revision to Diff 539818.Jul 12 2023, 7:05 PM

Make xray-attribute-instrumentation.ll less sensitive to .Ltmp/.Lxray_fn_idx label changes

Ami-zhang updated this revision to Diff 539836.Jul 12 2023, 8:52 PM

Remove unnecessary '\' symbol in xray-attribute-instrumentation.ll

This revision was automatically updated to reflect the committed changes.