This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Prefer to lower MC_GlobalAddress operands to .Lfoo$local
ClosedPublic

Authored by MaskRay on May 4 2021, 5:25 PM.

Details

Summary

Similar to X86 D73230 and AArch64 D101872

With this change, we can set dso_local in clang's -fpic -fno-semantic-interposition mode,
for default visibility external linkage non-ifunc-non-COMDAT definitions.

For such dso_local definitions, variable access/taking the address of a
function/calling a function will go through a local alias to avoid GOT/PLT.

Diff Detail

Event Timeline

MaskRay created this revision.May 4 2021, 5:25 PM
MaskRay requested review of this revision.May 4 2021, 5:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2021, 5:25 PM
MaskRay updated this revision to Diff 343242.May 5 2021, 5:04 PM

improve test

MaskRay updated this revision to Diff 343706.May 7 2021, 9:46 AM

add a function call test

MaskRay edited the summary of this revision. (Show Details)May 10 2021, 9:51 AM

Ping. This is an opt-in feature (D101876 clang -fpic -fno-semantic-interposition). No existing configurations are affected.

luismarques accepted this revision.May 10 2021, 2:09 PM
luismarques added a subscriber: aymanmus.

LGTM.

I assume the update_llc_test_checks.py issue that @aymanmus mentioned in D73228 was fixed a long while ago?

llvm/test/CodeGen/RISCV/elf-preemption.ll
2–4

Nit: we generally add both riscv32 and riscv64 (if applicable, generally naming the prefix appropriately). I'm not quite sure where to draw the line to ensure that doesn't become silly (@jrtc27 opinions?), but in this case it looks like the codegen might be the same for both subarchs, so it might not even require any additional clutter.
Also, split the long RUN lines, like in other tests.

139–140

Is a veneer the same thing as a thunk?

This revision is now accepted and ready to land.May 10 2021, 2:09 PM
jrtc27 added inline comments.May 10 2021, 2:12 PM
llvm/test/CodeGen/RISCV/elf-preemption.ll
2–4

GOT loads will differ in width, and stack spilling will differ in the few tests that end up doing that. RV32 tests should be redundant, but they should be so quick it does no harm to add them.

139–140

Veneer is the NIH term Arm use for range-extension thunks. This comment is meaningless for RISC-V; the bl is AArch64 and RISC-V has no veneers.

MaskRay updated this revision to Diff 344207.May 10 2021, 2:17 PM

reword a comment

MaskRay added inline comments.May 10 2021, 2:19 PM
llvm/test/CodeGen/RISCV/elf-preemption.ll
2–4

From the perspective of orthogonal tests, adding riscv32 provides very little value. The GOT load test has been done by other tests (e.g. pic-models.ll).

MaskRay marked 2 inline comments as done.May 10 2021, 2:19 PM
MaskRay added inline comments.
llvm/test/CodeGen/RISCV/elf-preemption.ll
139–140

I copied the comment from my aarch64 test. Fixed.

Still says bl

llvm/test/CodeGen/RISCV/elf-preemption.ll
139–140

(probably needs rewrapping)

jrtc27 added inline comments.May 10 2021, 2:21 PM
llvm/test/CodeGen/RISCV/elf-preemption.ll
2–4

Sure, but what's the downside of adding it? It's just two quick RUN lines. I take the view you should always test both unless there's a good reason not to, and laziness is not one of them.

MaskRay updated this revision to Diff 344208.May 10 2021, 2:21 PM
MaskRay marked an inline comment as done.

bl -> call

There are also Clang test failures that need to be addressed before committing.

There are also Clang test failures that need to be addressed before committing.

Actually, the failures were due to a bad commit in HEAD.

MaskRay marked 2 inline comments as done.May 11 2021, 9:28 AM
MaskRay added inline comments.
llvm/test/CodeGen/RISCV/elf-preemption.ll
2–4

Disagree with laziness.

The downside is the test time & readability. Duplicating RUN lines for nearly every test will unnecessarily increase test time.
CodeGen/RISCV, 886 files, 1029044 lines, lines/files is much larger than any other target.

There are dedicated tests for GOT. We should not add un-orthogonal tests to too many files.

MaskRay updated this revision to Diff 344439.May 11 2021, 9:29 AM
MaskRay marked an inline comment as done.

fix a typo

jrtc27 added inline comments.May 11 2021, 10:27 AM
llvm/test/CodeGen/RISCV/elf-preemption.ll
2–4

90% of which are RISC-V vector tests. These kinds of tiny tests are *not* the problem and take fractions of a second to run, it's the huge pile of vector tests.

2–4

But whether or not you're willing to add RV32 tests, you still need to split your RUN lines so they're formatted like all the others.

jrtc27 added inline comments.May 11 2021, 10:29 AM
llvm/test/CodeGen/RISCV/elf-preemption.ll
2–4

(917170 out of 1022002 lines, ie 89.7%, to be precise, in my checkout)

MaskRay added inline comments.May 11 2021, 10:35 AM
llvm/test/CodeGen/RISCV/elf-preemption.ll
2–4

But whether or not you're willing to add RV32 tests, you still need to split your RUN lines so they're formatted like all the others.

I don't understand. What's the style you prefer?

jrtc27 added inline comments.May 11 2021, 10:37 AM
llvm/test/CodeGen/RISCV/elf-preemption.ll
2–4

Like pic-models.ll:

; RUN: llc -mtriple=riscv32 -relocation-model=static < %s \
; RUN:     | FileCheck -check-prefix=RV32-STATIC %s
; RUN: llc -mtriple=riscv32 -relocation-model=pic < %s \
; RUN:     | FileCheck -check-prefix=RV32-PIC %s
; RUN: llc -mtriple=riscv64 -relocation-model=static < %s \
; RUN:     | FileCheck -check-prefix=RV64-STATIC %s
; RUN: llc -mtriple=riscv64 -relocation-model=pic < %s \
; RUN:     | FileCheck -check-prefix=RV64-PIC %s
MaskRay updated this revision to Diff 344478.May 11 2021, 10:44 AM

Use RV*-{STATIC,PIC}

jrtc27 added inline comments.May 11 2021, 10:45 AM
llvm/test/CodeGen/RISCV/elf-preemption.ll
2–4

But wrapped like that

10–21

Old CHECK lines

MaskRay updated this revision to Diff 344479.May 11 2021, 10:45 AM

remove old prefixes

Somehow your test has managed to lose the fact that some of the calls should be non-preemptible?

MaskRay updated this revision to Diff 344487.May 11 2021, 10:55 AM

fix tests

Somehow your test has managed to lose the fact that some of the calls should be non-preemptible?

Forgot ninja llc. Fixed.

Ok, but you _still_ haven't wrapped the RUN lines like the example I posted

Ok, but you _still_ haven't wrapped the RUN lines like the example I posted

Not sure this will improve readability. The longest RUN line just takes 99 characters. We don't enforce 80-character rule for tests.

Ok, but you _still_ haven't wrapped the RUN lines like the example I posted

Not sure this will improve readability. The longest RUN line just takes 99 characters. We don't enforce 80-character rule for tests.

We enforce this uniform formatting in the RISC-V tests. Even though the line is short, splitting it makes it much easier to compare each RUN line.

Ok, but you _still_ haven't wrapped the RUN lines like the example I posted

Not sure this will improve readability. The longest RUN line just takes 99 characters. We don't enforce 80-character rule for tests.

We enforce this uniform formatting in the RISC-V tests.

Quite a few rvv/ tests have 100+-character RUN lines.

(There is a style disagreement on where | is inserted. In many tests | is placed at the end of the previous line while here | is placed at the beginning of the continuation line)

Even though the line is short, splitting it makes it much easier to compare each RUN line.

To me it's different. Without interleaving wrapped FileCheck lines, it is easier to compare what options differ between two consecutive lines.

Ok, but you _still_ haven't wrapped the RUN lines like the example I posted

Not sure this will improve readability. The longest RUN line just takes 99 characters. We don't enforce 80-character rule for tests.

We enforce this uniform formatting in the RISC-V tests.

Quite a few rvv/ tests have 100+-character RUN lines.

Yes, rvv/ tests in both CodeGen and MC have ended up diverging because I wasn't reviewing them as vectors are not something I care much about except when they interact with memory.

(There is a style disagreement on where | is inserted. In many tests | is placed at the end of the previous line while here | is placed at the beginning of the continuation line)

All the ones directly under llvm/test/CodeGen/RISCV have the | on the next line. None of them have a single-line llc RUN line.

The only inconsistency is whether there are 1+2 spaces before the | (431 cases) or 1+4 spaces (32 cases). I guess pic-models.ll happens to be one of the odd ones out.

Even though the line is short, splitting it makes it much easier to compare each RUN line.

To me it's different. Without interleaving wrapped FileCheck lines, it is easier to compare what options differ between two consecutive lines.

We can debate personal preferences all we like, but that doesn't change the fact that there is a consistent style in the directory that this is going against.

jrtc27 accepted this revision.May 11 2021, 11:24 AM

Should probably be 3 spaces not 5 like I said given the above, but given that already exists in tree I'm not opposed to it. I might fix that though after this lands.

Ok, but you _still_ haven't wrapped the RUN lines like the example I posted

Not sure this will improve readability. The longest RUN line just takes 99 characters. We don't enforce 80-character rule for tests.

We enforce this uniform formatting in the RISC-V tests.

Quite a few rvv/ tests have 100+-character RUN lines.

Yes, rvv/ tests in both CodeGen and MC have ended up diverging because I wasn't reviewing them as vectors are not something I care much about except when they interact with memory.

(There is a style disagreement on where | is inserted. In many tests | is placed at the end of the previous line while here | is placed at the beginning of the continuation line)

All the ones directly under llvm/test/CodeGen/RISCV have the | on the next line. None of them have a single-line llc RUN line.

The only inconsistency is whether there are 1+2 spaces before the | (431 cases) or 1+4 spaces (32 cases). I guess pic-models.ll happens to be one of the odd ones out.

I picked +2 spaces, just because it is more common.

If you don't mind, I'd still want to switch to 2 spaces since it is more common elsewhere.

(If a new thing adds +4 spaces and I was a reviewer, I'd definitely raise the point before they go in.)

Even though the line is short, splitting it makes it much easier to compare each RUN line.

To me it's different. Without interleaving wrapped FileCheck lines, it is easier to compare what options differ between two consecutive lines.

We can debate personal preferences all we like, but that doesn't change the fact that there is a consistent style in the directory that this is going against.

Ok, but you _still_ haven't wrapped the RUN lines like the example I posted

Not sure this will improve readability. The longest RUN line just takes 99 characters. We don't enforce 80-character rule for tests.

We enforce this uniform formatting in the RISC-V tests.

Quite a few rvv/ tests have 100+-character RUN lines.

Yes, rvv/ tests in both CodeGen and MC have ended up diverging because I wasn't reviewing them as vectors are not something I care much about except when they interact with memory.

(There is a style disagreement on where | is inserted. In many tests | is placed at the end of the previous line while here | is placed at the beginning of the continuation line)

All the ones directly under llvm/test/CodeGen/RISCV have the | on the next line. None of them have a single-line llc RUN line.

The only inconsistency is whether there are 1+2 spaces before the | (431 cases) or 1+4 spaces (32 cases). I guess pic-models.ll happens to be one of the odd ones out.

I picked +2 spaces, just because it is more common.

No you used +4 spaces :)

If you don't mind, I'd still want to switch to 2 spaces since it is more common elsewhere.

(If a new thing adds +4 spaces and I was a reviewer, I'd definitely raise the point before they go in.)

See my comment above, which probably raced with yours :)

Should probably be 3 spaces not 5 like I said given the above, but given that already exists in tree I'm not opposed to it. I might fix that though after this lands.

I prefer +2 indentation (3 spaces) as well :) (RUN: |)

MaskRay updated this revision to Diff 344506.May 11 2021, 11:27 AM

+2 indentation (i.3. 3 spaces after colon) for continuation lines

jrtc27 accepted this revision.May 11 2021, 11:28 AM
This revision was landed with ongoing or failed builds.May 11 2021, 11:29 AM
This revision was automatically updated to reflect the committed changes.