⬆️ rust-analyzer

[matklad]

r? @ghost

[matklad]

@bors rollup=iffy
@bors r+

[bors]

📌 Commit 480f8e6 has been approved by matklad

[bors]

⌛ Testing commit 480f8e6 with merge 20a46d60e8f1773eb4fd6098f333cb4ba41e46b9...

[bors]

💔 Test failed - checks-actions

[matklad]

Urgh, so the failure seems legit, but it also looks like a bug in rustc/LLVM on riscv64:

    Compiling ra_ide v0.1.0 (/checkout/src/tools/rust-analyzer/crates/ra_ide)
[RUSTC-TIMING] ra_hir test:false 21.419
[RUSTC-TIMING] ra_ide_db test:false 23.395
[RUSTC-TIMING] ra_ssr test:false 23.368
[RUSTC-TIMING] ra_assists test:false 26.308
[RUSTC-TIMING] ra_hir_def test:false 40.207
LLVM ERROR: fixup value out of range
[RUSTC-TIMING] ra_hir_ty test:false 41.558
error: could not compile `ra_hir_ty`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
[RUSTC-TIMING] ra_ide test:false 30.755
[RUSTC-TIMING] rust_analyzer test:false 29.664
error: build failed

[jonas-schievink]

@msizanoen1 any idea what's happening here? It doesn't seem to happen locally (we're hitting linker errors, which means that LLVM has already run). Is CI using a different setup than what we get with rustup?

[msizanoen1]

Have you tried using the Docker containers?

./src/ci/docker/run.sh dist-riscv64-linux

will give you a build environment that is closest as possible to CI.

[msizanoen1]

@rustbot ping risc-v

[rustbot]

Error: Only Rust team members can ping teams.

Please let @rust-lang/release know if you're having trouble with this bot.

[msizanoen1]

@jonas-schievink Can you provide more information about the linker error?

[matklad]

@rustbot ping risc-v

(can I do this?)

Linker error looks like this:

ld.lld: error: /home/matklad/projects/rust-analyzer/target/riscv64gc-unknown-linux-gnu/release/deps/rust_analyzer-3619926d01f69cbd.10wond3dr24nwht7.rcgu.o is incompatible with elf_x86_64

which is understandable: what we did is just cargo build --target riscv64gc-unknown-linux-gnu, we haven't set up special linkers or anything. However, the hypothesis is that the error we observe on CI happens before the linker step, so that the linker error is unrelated.

[rustbot]

Error: This team (risc-v) cannot be pinged via this command;it may need to be added to triagebot.toml on the master branch.

Please let @rust-lang/release know if you're having trouble with this bot.

[msizanoen1]

Have you reproduced it using the Docker containers?
I tried to run it on this PR branch but it failed to reproduce.
I am retrying with the master changes merged and with the prebuilt Docker images from CI using rustup's ci/fetch-rust-docker.bash.

[matklad]

Have you reproduced it using the Docker containers?

No, I haven't tried that. Probably won't get to this myself short-term :(

[msizanoen1]

@matklad Even after merging master changes and using prebuilt images, I cannot reproduce this.
Can you retry the build?

[matklad]

@bors retry

[bors]

⌛ Testing commit 480f8e6 with merge 0f9caffa0b197d090635b3b760df644b6b220bfb...

[bors]

💔 Test failed - checks-actions

[msizanoen1]

I managed to reproduce it without docker using -C codegen-units=1.
This basically create functions so large that it creates out-of-range branch instructions within the function that LLVM can't handle.
Higher values for codegen-units split the crate into multiple objects and may reduce inlining and therefore function size.

[msizanoen1]

It should be possible to work-around it by reducing inlining e.g. by using lower optimization levels or more codegen units and it is possible to fix it in the LLVM backend by making it use more than one instructions for jump by loading the higher bits and the lower bits of the address separately and then do a jump instead of only using a single instruction when branching within a function.
cc @lenary (who works with the LLVM RISC-V backend)

[jonas-schievink]

Can reproduce locally as well. Even -C code-model=large does not fix it, so this clearly seems like an LLVM bug. In fact I also couldn't find a -C inline-threshold value small enough to fix this, but using 64 codegen units seem to work around the issue.

[lenary]

Urgh, that's a nasty bug. I'll see how much we can prioritize a fix for it.

[Mark-Simulacrum]

IMO, we should either temporarily disable rust analyzer for riscv or riscv entirely while that gets patched, blocking this on fixing riscv seems unreasonable given that it's presumably somewhat non-trivial to fix (requires at least LLVM updates).

[lenary]

Yes, it will need LLVM fixes, which will then need backporting. If you can disable rust-analyzer on the risc-v platform in the short term, that would be helpful.

[msizanoen1]

and it is possible to fix it in the LLVM backend by making it use more than one instructions for jump by loading the higher bits and the lower bits of the address separately and then do a jump instead of only using a single instruction when branching within a function.

LLVM already tries to do this in the BranchRelaxation pass but it seems like something after that pass still causes out of range branches.

[msizanoen1]

Also actually BranchRelaxation is not supported on RISC-V with -fPIC (it will throw a fatal error when it tries to create an indirect branch). This means the BranchRelaxation pass is somehow not triggered in rust-analyzer even if it contains out-of-range branches, which means the out-of-range branches was created after that pass.

[msizanoen1]

@lenary I believe that I have a fix for this:

diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index f6bc52968a3..32c09674ed4 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -386,9 +386,6 @@ unsigned RISCVInstrInfo::insertIndirectBranch(MachineBasicBlock &MBB,
   MachineRegisterInfo &MRI = MF->getRegInfo();
   const auto &TM = static_cast<const RISCVTargetMachine &>(MF->getTarget());
 
-  if (TM.isPositionIndependent())
-    report_fatal_error("Unable to insert indirect branch");
-
   if (!isInt<32>(BrOffset))
     report_fatal_error(
         "Branch offsets outside of the signed 32-bit range not supported");
@@ -398,16 +395,29 @@ unsigned RISCVInstrInfo::insertIndirectBranch(MachineBasicBlock &MBB,
   // uses the same workaround).
   Register ScratchReg = MRI.createVirtualRegister(&RISCV::GPRRegClass);
   auto II = MBB.end();
+  unsigned Scav;
+
+  if (TM.isPositionIndependent()) {
+    MBB.setLabelMustBeEmitted();
+    MachineInstr &AuipcMI = *BuildMI(MBB, II, DL, get(RISCV::AUIPC), ScratchReg)
+                                 .addMBB(&DestBB, RISCVII::MO_PCREL_HI);
+    BuildMI(MBB, II, DL, get(RISCV::PseudoBRIND))
+        .addReg(ScratchReg, RegState::Kill)
+        .addMBB(&MBB, RISCVII::MO_PCREL_LO);
+    RS->enterBasicBlockEnd(MBB);
+    Scav = RS->scavengeRegisterBackwards(RISCV::GPRRegClass,
+                                         AuipcMI.getIterator(), false, 0);
+  } else {
+    MachineInstr &LuiMI = *BuildMI(MBB, II, DL, get(RISCV::LUI), ScratchReg)
+                               .addMBB(&DestBB, RISCVII::MO_HI);
+    BuildMI(MBB, II, DL, get(RISCV::PseudoBRIND))
+        .addReg(ScratchReg, RegState::Kill)
+        .addMBB(&DestBB, RISCVII::MO_LO);
+    RS->enterBasicBlockEnd(MBB);
+    Scav = RS->scavengeRegisterBackwards(RISCV::GPRRegClass,
+                                         LuiMI.getIterator(), false, 0);
+  }
 
-  MachineInstr &LuiMI = *BuildMI(MBB, II, DL, get(RISCV::LUI), ScratchReg)
-                             .addMBB(&DestBB, RISCVII::MO_HI);
-  BuildMI(MBB, II, DL, get(RISCV::PseudoBRIND))
-      .addReg(ScratchReg, RegState::Kill)
-      .addMBB(&DestBB, RISCVII::MO_LO);
-
-  RS->enterBasicBlockEnd(MBB);
-  unsigned Scav = RS->scavengeRegisterBackwards(RISCV::GPRRegClass,
-                                                LuiMI.getIterator(), false, 0);
   MRI.replaceRegWith(ScratchReg, Scav);
   MRI.clearVirtRegs();
   RS->setRegUsed(Scav);
diff --git a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
index de71c01753d..9b1bd7f28ec 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -32,6 +32,11 @@
 #include "llvm/Target/TargetOptions.h"
 using namespace llvm;
 
+static cl::opt<bool>
+    BranchRelaxation("riscv-enable-branch-relax", cl::Hidden, cl::init(true),
+                     cl::desc("Relax out of range conditional branches"));
+
+
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVTarget() {
   RegisterTargetMachine<RISCVTargetMachine> X(getTheRISCV32Target());
   RegisterTargetMachine<RISCVTargetMachine> Y(getTheRISCV64Target());
@@ -126,7 +131,6 @@ public:
   bool addLegalizeMachineIR() override;
   bool addRegBankSelect() override;
   bool addGlobalInstructionSelect() override;
-  void addPreEmitPass() override;
   void addPreEmitPass2() override;
   void addPreRegAlloc() override;
 };
@@ -167,13 +171,14 @@ bool RISCVPassConfig::addGlobalInstructionSelect() {
   return false;
 }
 
-void RISCVPassConfig::addPreEmitPass() { addPass(&BranchRelaxationPassID); }
-
 void RISCVPassConfig::addPreEmitPass2() {
   // Schedule the expansion of AMOs at the last possible moment, avoiding the
   // possibility for other passes to break the requirements for forward
   // progress in the LR/SC block.
   addPass(createRISCVExpandPseudoPass());
+
+  if (BranchRelaxation)
+    addPass(&BranchRelaxationPassID);
 }
 
 void RISCVPassConfig::addPreRegAlloc() {

This patch does 2 things:

• Reorder the BranchRelaxation to be after the RISCVExpandPseudo
• Fix indirect branch generation on RISC-V with position independent code

I don't have much experience with LLVM so I would like to hear your review for this patch.

[lenary]

I'm not sure of the correctness of relaxing branches after the expand pseudo pass, as there are reasons the expand pseudo pass has to come last (due to the atomic instructions that are inserted).

Please can you file the patch on reviews.llvm.org and tag me as a reviewer (I use the same username there). If you point out this is a backport to fix a bug in LLVM 10 that should make it clearer to other reviewers that they shouldn't ask for a rebase (though we will need to forward-port this patch too).

[msizanoen1]

@matklad Until LLVM is fixed, can you remove the .expect("rust-analyzer always builds") and continue to merge?

[matklad]

I don't think we should use remove the expect, rather, we should skip the build on risc-v entirely. I'll try too look into that some time soonish

[msizanoen1]

@lenary Assuming that values here are correct, instead of pass reordering we can simply backport this to https://github.com/rust-lang/llvm-project fork.
The indirect branch implementation for PIC is needed in all cases.
(I was unable to access reviews.llvm.org currently, it just throws some MySQL Error. I will check back later.)

[lenary]

Yes, the PIC indirect branch implementation is needed in all cases, I agree.

I also agree that branch relaxation is needed after expanding some pseudos. In trunk LLVM we split the expand pseudos so we dealt with the atomic ones separately to the non-atomic (mostly branchy) ones - this means we can insert branch relaxation between them, leaving atomic pseudo expansion to last, which makes it easier to prove forward progress guarantees needed for the atomic expansions.

[lenary]

Update on patching the issue:

• LLVM 10.0.1 has part of the fix (more accurate code sizes for the pseudo-instructions)
• @msizanoen1 is preparing the fix for long indirect jumps in PIC-mode on RV64. This patch is under review, and solves bugs we've seen in other places.