[RISCV] Allow mismatched SmallDataLimit and use Min for conflicting values

[hiraditya]

Summary

Authored by: @joshua-arch1

This patch is to fix an issue about module linking with LTO.

When compiling with PIE, the small data limitation needs to be consistent with that in PIC, otherwise there will be linking errors due to conflicting values.

bar.c

int bar() { return 1; }

foo.c

int foo() { return 1; }

clang --target=riscv64-unknown-linux-gnu -flto -c foo.c -o foo.o -fPIE
clang --target=riscv64-unknown-linux-gnu -flto -c bar.c -o bar.o -fPIC

clang --target=riscv64-unknown-linux-gnu -flto foo.o bar.o -flto -nostdlib -v -fuse-ld=lld

ld.lld: error: linking module flags 'SmallDataLimit': IDs have conflicting values in 'bar.o' and 'ld-temp.o'
clang-15: error: linker command failed with exit code 1 (use -v to see invocation)

What we are trying to do here is to use Min instead of Error for conflicting SmallDataLimit when combining -fno-PIC code with -fPIC code.

Signed-off-by: xiaojing.zhang <[e-mail]>
Signed-off-by: jianxin.lai <[e-mail]>

[jrtc27]

I don’t understand. PIEs can use sdata just fine.

[jrtc27]

And why would the existence of .sdata lead to linker errors?

[jrtc27]

So your error is about _module_ linking with LTO, not LLD. In that case this doesn't fix the problem, you can still hit it by combining -fno-PIC code with -fPIC code. The correct thing to do is allow mismatched SmallDataLimit and merge them into something sensible (either min or max).

[enh]

@MaskRay for his opinion...

[jrtc27]

Description should say LTO in the first paragraph as otherwise it sounds like normal ELF linking not LLVM module linking. Otherwise the diff looks sensible.

[joshua-arch1]

In D131230#3904807, @jrtc27 wrote:

Description should say LTO in the first paragraph as otherwise it sounds like normal ELF linking not LLVM module linking. Otherwise the diff looks sensible.

The description has been updated.

[joshua-arch1]

In D131230#3904807, @jrtc27 wrote:

Description should say LTO in the first paragraph as otherwise it sounds like normal ELF linking not LLVM module linking. Otherwise the diff looks sensible.

Ping.

[MaskRay]

Sorry for my belated response. D57497 introduced the Module Flags Metadata "SmallDataLimit". It defaults to 0 for -fPIC and (with some exceptions) otherwise 8.
So a -fno-pic/-fPIE bitcode file and a -fPIC bitcode file cannot be mixed together because "SmallDataLimit" uses the merge behavior Error (all values must match).
There is an overly restricted condition. I agree that lifting it to Min (which is a fairly new addition) makes sense, although I am unsure why SmallDataLimit needs to have a default value of 8 in the first place...

Be sure that @jrtc27 is happy as well.

clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c

40–41

If I saw D57497 earlier, I'd suggest adding a prefix for every unique value, instead of using a unique prefix for every test. But you don't need to simplify this.

[hiraditya]

Is it okay to merge this patch?

[hiraditya]

cc: @jrtc27 anything you still want addressed?

[MaskRay]

In D131230#4086839, @hiraditya wrote:

Is it okay to merge this patch?

Yes, seems fine to me.

[hiraditya]

@joshua-arch1 can you share your email such that I can commit this with proper author info? Feel free to ping me directly on discord: Aditya Kumar#9439

[dyung]

@joshua-arch1 and @hiraditya this change is causing a test failure on many bots. Can you take a look and revert if you need time to investigate?

Failing bots:

• https://lab.llvm.org/buildbot/#/builders/183/builds/10688
• https://lab.llvm.org/buildbot/#/builders/249/builds/349
• https://lab.llvm.org/buildbot/#/builders/231/builds/8162
• https://lab.llvm.org/buildbot/#/builders/121/builds/27741
• https://lab.llvm.org/buildbot/#/builders/230/builds/8973
• https://lab.llvm.org/buildbot/#/builders/57/builds/24514
• https://lab.llvm.org/buildbot/#/builders/109/builds/57335
• https://lab.llvm.org/buildbot/#/builders/247/builds/1342
• https://lab.llvm.org/buildbot/#/builders/36/builds/30143

[• pcwang-thead]

I have reverted it in 3df16e6f6e4d933f3839003e29b8a4b70e4c7ec8.
Please update CodeGen/RISCV/rvv-intrinsics-handcrafted/vlenb.c and reland this patch again.

[hiraditya]

thanks for reverting it. i'll take care of the test failure.

[hiraditya]

reopening to update the failing testcase.

[hiraditya]

Updated the failing testcase.

[MaskRay]

I updated more i32 1, !"SmallDataLimit" and re-landed this patch.

clang/lib/Driver/ToolChains/Clang.cpp

 
 static void SetRISCVSmallDataLimit(const ToolChain &TC, const ArgList &Args,
                                    ArgStringList &CmdArgs) {
   const Driver &D = TC.getDriver();
   const llvm::Triple &Triple = TC.getTriple();
   // Default small data limitation is eight.
   const char *SmallDataLimit = "8";
   // Get small data limitation.
-  if (Args.getLastArg(options::OPT_shared, options::OPT_fpic,
-                      options::OPT_fPIC)) {
-    // Not support linker relaxation for PIC.
+  if (Args.getLastArg(options::OPT_shared,
+                      options::OPT_fpic, options::OPT_fPIC,
+                      options::OPT_fpie, options::OPT_fPIE)) {
+    // Not support linker relaxation for PIC and PIE.
     SmallDataLimit = "0";
     if (Args.hasArg(options::OPT_G)) {
       D.Diag(diag::warn_drv_unsupported_sdata);
     }
   } else if (Args.getLastArgValue(options::OPT_mcmodel_EQ)
                  .equals_insensitive("large") &&
              (Triple.getArch() == llvm::Triple::riscv64)) {
     // Not support linker relaxation for RV64 with large code model.

clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c

 // RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -o - \
 // RUN:   | FileCheck %s -check-prefix=RV32-DEFAULT
 // RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -G4 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV32-G4
 // RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -msmall-data-limit=0 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV32-S0
 // RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -msmall-data-limit=2 -G4 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV32-S2G4
 // RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -msmall-data-threshold=16 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV32-T16
 // RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -fpic -o - \
 // RUN:   | FileCheck %s -check-prefix=RV32-PIC
+// RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -fpie -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32-PIE
 
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-DEFAULT
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -G4 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-G4
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -msmall-data-limit=0 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-S0
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -msmall-data-limit=2 -G4 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-S2G4
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -msmall-data-threshold=16 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-T16
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -fpic -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-PIC
+// RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -fpie -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64-PIE
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -mcmodel=large -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-LARGE
 
 void test(void) {}
 
 // RV32-DEFAULT: !{i32 1, !"SmallDataLimit", i32 8}
 // RV32-G4:      !{i32 1, !"SmallDataLimit", i32 4}
 // RV32-S0:      !{i32 1, !"SmallDataLimit", i32 0}
 // RV32-S2G4:    !{i32 1, !"SmallDataLimit", i32 4}
 // RV32-T16:     !{i32 1, !"SmallDataLimit", i32 16}
 // RV32-PIC:     !{i32 1, !"SmallDataLimit", i32 0}
+// RV32-PIE:     !{i32 1, !"SmallDataLimit", i32 0}
 
 // RV64-DEFAULT: !{i32 1, !"SmallDataLimit", i32 8}
 // RV64-G4:      !{i32 1, !"SmallDataLimit", i32 4}
 // RV64-S0:      !{i32 1, !"SmallDataLimit", i32 0}
 // RV64-S2G4:    !{i32 1, !"SmallDataLimit", i32 4}
 // RV64-T16:     !{i32 1, !"SmallDataLimit", i32 16}
 // RV64-PIC:     !{i32 1, !"SmallDataLimit", i32 0}
 // RV64-LARGE:   !{i32 1, !"SmallDataLimit", i32 0}
+// RV64-PIE:     !{i32 1, !"SmallDataLimit", i32 0}
 
 // The value will be passed by module flag instead of target feature.
 // RV32-S0-NOT: +small-data-limit=
 // RV64-S0-NOT: +small-data-limit=