Skip to content

Commit e640d9e

Browse files
authored
[RISCV][GlobalISel] Fix legalizing ‘llvm.va_copy’ intrinsic (#86863)
Hi, I spotted a problem when running benchmarking programs on a RISCV64 device. ## Issue Segmentation faults only occurred while running the programs compiled with `GlobalISel` enabled. Here is a small but complete example (it is adopted from [Google's benchmark framework](https://github.com/llvm/llvm-test-suite/blob/95a9f0d0b45056274f0bb4b0e0dd019023e414dc/MicroBenchmarks/libs/benchmark/src/colorprint.cc#L85-L119) to reproduce the issue, ```cpp #include <cstdarg> #include <cstdio> #include <iostream> #include <memory> #include <string> std::string FormatString(const char* msg, va_list args) { // we might need a second shot at this, so pre-emptivly make a copy va_list args_cp; va_copy(args_cp, args); std::size_t size = 256; char local_buff[256]; auto ret = vsnprintf(local_buff, size, msg, args_cp); va_end(args_cp); // currently there is no error handling for failure, so this is hack. // BM_CHECK(ret >= 0); if (ret == 0) // handle empty expansion return {}; else if (static_cast<size_t>(ret) < size) return local_buff; else { // we did not provide a long enough buffer on our first attempt. size = static_cast<size_t>(ret) + 1; // + 1 for the null byte std::unique_ptr<char[]> buff(new char[size]); ret = vsnprintf(buff.get(), size, msg, args); // BM_CHECK(ret > 0 && (static_cast<size_t>(ret)) < size); return buff.get(); } } std::string FormatString(const char* msg, ...) { va_list args; va_start(args, msg); auto tmp = FormatString(msg, args); va_end(args); return tmp; } int main() { std::string Str = FormatString("%-*s %13s %15s %12s", static_cast<int>(20), "Benchmark", "Time", "CPU", "Iterations"); std::cout << Str << std::endl; } ``` Use `clang++ -fglobal-isel -o main main.cpp` to compile it. ## Cause I have examined MIR, it shows that these segmentation faults resulted from a small mistake about legalizing the intrinsic function `llvm.va_copy`. https://github.com/llvm/llvm-project/blob/36e74cfdbde208e384c72bcb52ea638303fb7d67/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp#L451-L453 `DstLst` and `Tmp` are placed in the wrong order. ## Changes I have tweaked the test case `CodeGen/RISCV/GlobalISel/vararg.ll` so that `s0` is used as the frame pointer (not in all checks) which points to the starting address of the save area. I believe that it helps reason about how `llvm.va_copy` is handled.
1 parent 856e815 commit e640d9e

File tree

3 files changed

+933
-7
lines changed

3 files changed

+933
-7
lines changed

llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,7 @@ bool RISCVLegalizerInfo::legalizeIntrinsic(LegalizerHelper &Helper,
450450
// Store the result in the destination va_list
451451
MachineMemOperand *StoreMMO = MF.getMachineMemOperand(
452452
MachinePointerInfo(), MachineMemOperand::MOStore, PtrTy, Alignment);
453-
MIRBuilder.buildStore(DstLst, Tmp, *StoreMMO);
453+
MIRBuilder.buildStore(Tmp, DstLst, *StoreMMO);
454454

455455
MI.eraseFromParent();
456456
return true;

llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-vacopy.mir

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ body: |
1414
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(p0) = COPY $x10
1515
; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(p0) = COPY $x11
1616
; CHECK-NEXT: [[LOAD:%[0-9]+]]:_(p0) = G_LOAD [[COPY1]](p0) :: (load (p0))
17-
; CHECK-NEXT: G_STORE [[COPY]](p0), [[LOAD]](p0) :: (store (p0))
17+
; CHECK-NEXT: G_STORE [[LOAD]](p0), [[COPY]](p0) :: (store (p0))
1818
; CHECK-NEXT: PseudoRET
1919
%0:_(p0) = COPY $x10
2020
%1:_(p0) = COPY $x11

0 commit comments

Comments
 (0)