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

[MaskRay]

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.

[MaskRay]

improve test

[MaskRay]

add a function call test

[MaskRay]

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

[luismarques]

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

1–3 ↗

(On Diff #343706)

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.

138–139 ↗

(On Diff #343706)

Is a veneer the same thing as a thunk?

[jrtc27]

llvm/test/CodeGen/RISCV/elf-preemption.ll

1–3 ↗

(On Diff #343706)

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.

138–139 ↗

(On Diff #343706)

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]

reword a comment

[MaskRay]

llvm/test/CodeGen/RISCV/elf-preemption.ll

1–3 ↗

(On Diff #343706)

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]

llvm/test/CodeGen/RISCV/elf-preemption.ll

138–139 ↗

(On Diff #343706)

I copied the comment from my aarch64 test. Fixed.

[jrtc27]

Still says bl

llvm/test/CodeGen/RISCV/elf-preemption.ll

138–139 ↗

(On Diff #344207)

(probably needs rewrapping)

[jrtc27]

llvm/test/CodeGen/RISCV/elf-preemption.ll

1–3 ↗

(On Diff #343706)

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]

bl -> call

[luismarques]

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

[luismarques]

In D101875#2750391, @luismarques wrote:

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]

llvm/test/CodeGen/RISCV/elf-preemption.ll

1–3 ↗

(On Diff #343706)

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]

fix a typo

[jrtc27]

llvm/test/CodeGen/RISCV/elf-preemption.ll

1–3 ↗

(On Diff #343706)

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.

1–3 ↗

(On Diff #343706)

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]

llvm/test/CodeGen/RISCV/elf-preemption.ll

1–3 ↗

(On Diff #343706)

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

[MaskRay]

llvm/test/CodeGen/RISCV/elf-preemption.ll

1–3 ↗

(On Diff #343706)

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]

llvm/test/CodeGen/RISCV/elf-preemption.ll

1–3 ↗

(On Diff #343706)

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]

Use RV*-{STATIC,PIC}

[jrtc27]

llvm/test/CodeGen/RISCV/elf-preemption.ll

9–20 ↗

(On Diff #344478)

Old CHECK lines

1–3 ↗

(On Diff #343706)

But wrapped like that

[MaskRay]

remove old prefixes

[jrtc27]

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

[MaskRay]

fix tests

[MaskRay]

In D101875#2751349, @jrtc27 wrote:

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

Forgot ninja llc. Fixed.

[jrtc27]

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

[MaskRay]

In D101875#2751397, @jrtc27 wrote:

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.

[jrtc27]

In D101875#2751443, @MaskRay wrote:

In D101875#2751397, @jrtc27 wrote:

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.

[MaskRay]

In D101875#2751448, @jrtc27 wrote:

In D101875#2751443, @MaskRay wrote:

In D101875#2751397, @jrtc27 wrote:

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.

[jrtc27]

In D101875#2751459, @MaskRay wrote:

In D101875#2751448, @jrtc27 wrote:

In D101875#2751443, @MaskRay wrote:

In D101875#2751397, @jrtc27 wrote:

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.

[MaskRay]

style

[jrtc27]

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.

[MaskRay]

In D101875#2751502, @jrtc27 wrote:

In D101875#2751459, @MaskRay wrote:

In D101875#2751448, @jrtc27 wrote:

In D101875#2751443, @MaskRay wrote:

In D101875#2751397, @jrtc27 wrote:

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.

[jrtc27]

In D101875#2751558, @MaskRay wrote:

In D101875#2751502, @jrtc27 wrote:

In D101875#2751459, @MaskRay wrote:

In D101875#2751448, @jrtc27 wrote:

In D101875#2751443, @MaskRay wrote:

In D101875#2751397, @jrtc27 wrote:

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 :)

[MaskRay]

In D101875#2751554, @jrtc27 wrote:

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]

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

llvm/lib/Target/RISCV/RISCVMCInstLower.cpp

case MachineOperand::MO_RegisterMask:
     return false;
   case MachineOperand::MO_Immediate:
     MCOp = MCOperand::createImm(MO.getImm());
     break;
   case MachineOperand::MO_MachineBasicBlock:
     MCOp = lowerSymbolOperand(MO, MO.getMBB()->getSymbol(), AP);
     break;
   case MachineOperand::MO_GlobalAddress:
-    MCOp = lowerSymbolOperand(MO, AP.getSymbol(MO.getGlobal()), AP);
+    MCOp = lowerSymbolOperand(MO, AP.getSymbolPreferLocal(*MO.getGlobal()), AP);
     break;
   case MachineOperand::MO_BlockAddress:
     MCOp = lowerSymbolOperand(
         MO, AP.GetBlockAddressSymbol(MO.getBlockAddress()), AP);
     break;
   case MachineOperand::MO_ExternalSymbol:
     MCOp = lowerSymbolOperand(
         MO, AP.GetExternalSymbolSymbol(MO.getSymbolName()), AP);

llvm/test/CodeGen/RISCV/pic-models.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; 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
 
 ; Check basic lowering of PIC addressing.
 ; TODO: Check other relocation models?
 
 @external_var = external global i32
 @internal_var = internal global i32 42
+@dsolocal_var = dso_local global i32 42
 
 
 ; external address
 
 define i32* @f1() nounwind {
 ; RV32-STATIC-LABEL: f1:
 ; RV32-STATIC:       # %bb.0: # %entry
 ; RV32-STATIC-NEXT:    lui a0, %hi(external_var)

 ; RV64-PIC-NEXT:  .LBB1_1: # %entry
 ; RV64-PIC-NEXT:    # Label of block must be emitted
 ; RV64-PIC-NEXT:    auipc a0, %pcrel_hi(internal_var)
 ; RV64-PIC-NEXT:    addi a0, a0, %pcrel_lo(.LBB1_1)
 ; RV64-PIC-NEXT:    ret
 entry:
   ret i32* @internal_var
 }
+
+; default visibility dso_local external linkage
+
+define i32* @f3() nounwind {
+; RV32-STATIC-LABEL: f3:
+; RV32-STATIC:       # %bb.0: # %entry
+; RV32-STATIC-NEXT:    lui a0, %hi(dsolocal_var)
+; RV32-STATIC-NEXT:    addi a0, a0, %lo(dsolocal_var)
+; RV32-STATIC-NEXT:    ret
+;
+; RV32-PIC-LABEL: f3:
+; RV32-PIC:       # %bb.0: # %entry
+; RV32-PIC-NEXT:  .LBB2_1: # %entry
+; RV32-PIC-NEXT:    # Label of block must be emitted
+; RV32-PIC-NEXT:    auipc a0, %pcrel_hi(.Ldsolocal_var$local)
+; RV32-PIC-NEXT:    addi a0, a0, %pcrel_lo(.LBB2_1)
+; RV32-PIC-NEXT:    ret
+;
+; RV64-STATIC-LABEL: f3:
+; RV64-STATIC:       # %bb.0: # %entry
+; RV64-STATIC-NEXT:    lui a0, %hi(dsolocal_var)
+; RV64-STATIC-NEXT:    addi a0, a0, %lo(dsolocal_var)
+; RV64-STATIC-NEXT:    ret
+;
+; RV64-PIC-LABEL: f3:
+; RV64-PIC:       # %bb.0: # %entry
+; RV64-PIC-NEXT:  .LBB2_1: # %entry
+; RV64-PIC-NEXT:    # Label of block must be emitted
+; RV64-PIC-NEXT:    auipc a0, %pcrel_hi(.Ldsolocal_var$local)
+; RV64-PIC-NEXT:    addi a0, a0, %pcrel_lo(.LBB2_1)
+; RV64-PIC-NEXT:    ret
+entry:
+  ret i32* @dsolocal_var
+}