This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Encode LoongArch specific ELF e_flags to binary by LoongArchTargetStreamer
ClosedPublic

Authored by SixWeining on Jul 21 2022, 3:02 AM.

Details

Summary

Reference: https://github.com/loongson/LoongArch-Documentation
The last commit hash (main branch) is:
99016636af64d02dee05e39974d4c1e55875c45b

Note:
There are several PRs that affect the e_flags.
After they got closed or merged, we should update the implementation here accordingly.

Diff Detail

Event Timeline

SixWeining created this revision.Jul 21 2022, 3:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 3:02 AM
SixWeining requested review of this revision.Jul 21 2022, 3:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 3:02 AM
MaskRay added inline comments.Jul 22 2022, 12:08 PM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchELFStreamer.cpp
42

delete blank line after EFlags

it's fine if you think the EFlags is different from the previous 3 variables and inserts a blank line before.

76

// end namespace

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCTargetDesc.cpp
86

avoid the used once variable

124

Below you use end namespace. Be consistent.

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchTargetStreamer.h
18

delete blank line

llvm/test/CodeGen/LoongArch/e_flags.ll
2
llc ... -o %t
llvm-readelf -h %t | FileCheck ...

We are transitioning into using temporary files for some intermediate commands when they can be useful for debugging.

11

Use single ; for RUN and CHECK lines.

SixWeining marked 5 inline comments as done.

Address @MaskRay's comments.

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCTargetDesc.cpp
124

Thanks.

And another issue: 2 formats are used currently:

  1. // end xxx namespace
  2. // namespace xxx

Let me fix it in a seperate NFC patch.

llvm/test/CodeGen/LoongArch/e_flags.ll
11

Thanks.

use // end namespace xxx style comment

xen0n added a comment.Jul 26 2022, 1:09 AM

Do we have to wait before https://github.com/loongson/LoongArch-Documentation/pull/33 is merged (and perhaps the reference implementation in binutils) so we could mark LLVM-generated objects as such? The LLVM LoongArch port can never generate the old-style (stack-machine-style) relocs so I think this marker might be appropriate at all times.

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchELFStreamer.cpp
27

nit: "appropriate"

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchTargetStreamer.cpp
23

In LoongArchELFStreamer.cpp you wrote "initialised". Do we prefer British or American spelling here in LLVM?

SixWeining marked 2 inline comments as done.

typo fix

Do we have to wait before https://github.com/loongson/LoongArch-Documentation/pull/33 is merged (and perhaps the reference implementation in binutils) so we could mark LLVM-generated objects as such? The LLVM LoongArch port can never generate the old-style (stack-machine-style) relocs so I think this marker might be appropriate at all times.

Thanks. Let me look at that discussion and reply later.

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchELFStreamer.cpp
27

Thanks.

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchTargetStreamer.cpp
23

Seems initialized is used more frequently in LLVM.

Do we have to wait before https://github.com/loongson/LoongArch-Documentation/pull/33 is merged (and perhaps the reference implementation in binutils) so we could mark LLVM-generated objects as such? The LLVM LoongArch port can never generate the old-style (stack-machine-style) relocs so I think this marker might be appropriate at all times.

Yes, we do not plan to generate the old-style (stack-machine-style) relocs in LLVM that means we are targeting the socalled new-world. But that PR you mentioned has been open 8 months and I'm not sure when it would be merged, plus this one . Maybe we can ignore them for now and leave a NOTE or TODO here so that we can move on to implement the other parts (relocs, ABI lowering in clang, inlineasm...)?

xen0n added a comment.Jul 27 2022, 6:26 PM

Do we have to wait before https://github.com/loongson/LoongArch-Documentation/pull/33 is merged (and perhaps the reference implementation in binutils) so we could mark LLVM-generated objects as such? The LLVM LoongArch port can never generate the old-style (stack-machine-style) relocs so I think this marker might be appropriate at all times.

Yes, we do not plan to generate the old-style (stack-machine-style) relocs in LLVM that means we are targeting the socalled new-world. But that PR you mentioned has been open 8 months and I'm not sure when it would be merged, plus this one . Maybe we can ignore them for now and leave a NOTE or TODO here so that we can move on to implement the other parts (relocs, ABI lowering in clang, inlineasm...)?

I mentioned the PR because I saw activity from your team, leading to the conclusion that the PR would eventually get merged in a relatively short time. Meanwhile, for now there is no pressing reason to mark generated objects as such, because at present no tooling exists without the old-style relocs support; a NOTE or TODO works for me. Thanks.

SixWeining edited the summary of this revision. (Show Details)Jul 27 2022, 7:22 PM

Do we have to wait before https://github.com/loongson/LoongArch-Documentation/pull/33 is merged (and perhaps the reference implementation in binutils) so we could mark LLVM-generated objects as such? The LLVM LoongArch port can never generate the old-style (stack-machine-style) relocs so I think this marker might be appropriate at all times.

Yes, we do not plan to generate the old-style (stack-machine-style) relocs in LLVM that means we are targeting the socalled new-world. But that PR you mentioned has been open 8 months and I'm not sure when it would be merged, plus this one . Maybe we can ignore them for now and leave a NOTE or TODO here so that we can move on to implement the other parts (relocs, ABI lowering in clang, inlineasm...)?

I mentioned the PR because I saw activity from your team, leading to the conclusion that the PR would eventually get merged in a relatively short time. Meanwhile, for now there is no pressing reason to mark generated objects as such, because at present no tooling exists without the old-style relocs support; a NOTE or TODO works for me. Thanks.

Thanks. I just update the summary to reflect this.

SixWeining edited the summary of this revision. (Show Details)Aug 14 2022, 10:14 PM

@xen0n Do you think we can merge this now so that to unblock the relocs implementation?

And if PR #33, PR #47 and PR #61 get merged, let us update the implementation here accordingly.

xen0n accepted this revision.Aug 14 2022, 10:18 PM

@xen0n Do you think we can merge this now so that to unblock the relocs implementation?

And if PR #33, PR #47 and PR #61 get merged, let us update the implementation here accordingly.

Hmm yes let's go ahead for the time being. It's early construction phase so global patching of object files or even world recompilations is well within reach.

This revision is now accepted and ready to land.Aug 14 2022, 10:18 PM
This revision was landed with ongoing or failed builds.Aug 15 2022, 10:56 PM
This revision was automatically updated to reflect the committed changes.