Skip to content

Backport [TailCallElim] Remove the readonly attribute of byval. #150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1103,12 +1103,6 @@ WebAssembly Support
^^^^^^^^^^^^^^^^^^^
- The -mcpu=generic configuration now enables sign-ext and mutable-globals. These
proposals are standardized and available in all major engines.
- Shared library support (and PIC code generation) for WebAssembly is no longer
limited to the Emscripten target OS and now works with other targets such as
wasm32-wasi. Note that the `format
<https://github.com/WebAssembly/tool-conventions/blob/main/DynamicLinking.md>`_
is not yet stable and may change between LLVM versions. Also, WASI does not
yet have facilities to load dynamic libraries.

DWARF Support in Clang
----------------------
Expand Down
5 changes: 1 addition & 4 deletions clang/lib/Driver/ToolChains/WebAssembly.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,13 @@ void wasm::Linker::ConstructJob(Compilation &C, const JobAction &JA,
<< CM << A->getOption().getName();
}
}
if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles, options::OPT_shared))
if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles))
CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath(Crt1)));
if (Entry) {
CmdArgs.push_back(Args.MakeArgString("--entry"));
CmdArgs.push_back(Args.MakeArgString(Entry));
}

if (Args.hasArg(options::OPT_shared))
CmdArgs.push_back(Args.MakeArgString("-shared"));

AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs, JA);

if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
Expand Down
26 changes: 0 additions & 26 deletions clang/test/Driver/wasm-toolchain.c
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,6 @@
// LINK_KNOWN: "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
// LINK_KNOWN: wasm-ld{{.*}}" "-L/foo/lib/wasm32-wasi" "crt1.o" "[[temp]]" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"

// -shared should be passed through to `wasm-ld` and not include crt1.o with a known OS.

// RUN: %clang -### -shared --target=wasm32-wasi --sysroot=/foo %s 2>&1 \
// RUN: | FileCheck -check-prefix=LINK_KNOWN_SHARED %s
// LINK_KNOWN_SHARED: "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
// LINK_KNOWN_SHARED: wasm-ld{{.*}}" "-L/foo/lib/wasm32-wasi" "-shared" "[[temp]]" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"

// -shared should be passed through to `wasm-ld` and not include crt1.o with an unknown OS.

// RUN: %clang -### -shared --target=wasm32-unknown-unknown --sysroot=/foo %s 2>&1 \
// RUN: | FileCheck -check-prefix=LINK_UNKNOWN_SHARED %s
// LINK_UNKNOWN_SHARED: "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
// LINK_UNKNOWN_SHARED: wasm-ld{{.*}}" "-shared" "[[temp]]" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"

// A basic C link command-line with optimization with known OS.

// RUN: %clang -### -O2 --target=wasm32-wasi --sysroot=/foo %s 2>&1 \
Expand All @@ -60,18 +46,6 @@
// RUN: | FileCheck -check-prefix=COMPILE %s
// COMPILE: "-cc1" {{.*}} "-internal-isystem" "/foo/include/wasm32-wasi" "-internal-isystem" "/foo/include"

// -fPIC should work on a known OS

// RUN: %clang -### -fPIC --target=wasm32-wasi --sysroot=/foo %s 2>&1 \
// RUN: | FileCheck -check-prefix=COMPILE_KNOWN_PIC %s
// COMPILE_KNOWN_PIC: "-cc1" {{.*}} "-mrelocation-model" "pic" "-pic-level" "2" {{.*}} "-internal-isystem" "/foo/include/wasm32-wasi" "-internal-isystem" "/foo/include"

// -fPIC should work on an unknown OS

// RUN: %clang -### -fPIC --target=wasm32-unknown-unknown --sysroot=/foo %s 2>&1 \
// RUN: | FileCheck -check-prefix=COMPILE_UNKNOWN_PIC %s
// COMPILE_UNKNOWN_PIC: "-cc1" {{.*}} "-mrelocation-model" "pic" "-pic-level" "2"

// Thread-related command line tests.

// '-pthread' sets +atomics, +bulk-memory, +mutable-globals, +sign-ext, and --shared-memory
Expand Down
7 changes: 7 additions & 0 deletions llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ static Reloc::Model getEffectiveRelocModel(std::optional<Reloc::Model> RM,
return Reloc::Static;
}

if (!TT.isOSEmscripten()) {
// Relocation modes other than static are currently implemented in a way
// that only works for Emscripten, so disable them if we aren't targeting
// Emscripten.
return Reloc::Static;
}

return *RM;
}

Expand Down
6 changes: 6 additions & 0 deletions llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,12 @@ bool TailRecursionEliminator::eliminateCall(CallInst *CI) {
for (unsigned I = 0, E = CI->arg_size(); I != E; ++I) {
if (CI->isByValArgument(I)) {
copyLocalTempOfByValueOperandIntoArguments(CI, I);
// When eliminating a tail call, we modify the values of the arguments.
// Therefore, if the byval parameter has a readonly attribute, we have to
// remove it. It is safe because, from the perspective of a caller, the
// byval parameter is always treated as "readonly," even if the readonly
// attribute is removed.
F.removeParamAttr(I, Attribute::ReadOnly);
ArgumentPHIs[I]->addIncoming(F.getArg(I), BB);
} else
ArgumentPHIs[I]->addIncoming(CI->getArgOperand(I), BB);
Expand Down
27 changes: 27 additions & 0 deletions llvm/test/Transforms/PhaseOrdering/pr64289-tce.ll
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -S -O3 < %s | FileCheck %s

; A miscompilation found on https://github.com/llvm/llvm-project/issues/64289.
; 1. PostOrderFunctionAttrsPass added readonly to the parameter.
; 2. TailCallElimPass modified the parameter but kept readonly.
; 3. LICMPass incorrectly hoisted the load instruction.

define void @pr64289(ptr noalias byval(i64) %x) {
; CHECK-LABEL: @pr64289(
; CHECK-NEXT: start:
; CHECK-NEXT: ret void
;
start:
%new_x = alloca i64, align 8
%x_val = load i64, ptr %x, align 8
%is_zero = icmp eq i64 %x_val, 0
br i1 %is_zero, label %end, label %recurse

recurse:
store i64 0, ptr %new_x, align 8
call void @pr64289(ptr %new_x)
br label %end

end:
ret void
}
17 changes: 9 additions & 8 deletions llvm/test/Transforms/TailCallElim/tre-byval-parameter-2.ll
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature
; RUN: opt < %s -passes=tailcallelim -verify-dom-info -S | FileCheck %s

; the test was generated from the following C++ source:
Expand All @@ -24,23 +24,24 @@

; Function Attrs: noinline nounwind uwtable
define dso_local void @_Z7dostuff1AS_i(ptr nocapture byval(%struct.A) align 8 %a, ptr nocapture readonly byval(%struct.A) align 8 %b, i32 %i) local_unnamed_addr #0 {
; CHECK-LABEL: @_Z7dostuff1AS_i(
; CHECK-LABEL: define {{[^@]+}}@_Z7dostuff1AS_i
; CHECK-SAME: (ptr nocapture byval([[STRUCT_A:%.*]]) align 8 [[A:%.*]], ptr nocapture byval([[STRUCT_A]]) align 8 [[B:%.*]], i32 [[I:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
; CHECK-NEXT: entry:
; CHECK-NEXT: [[AGG_TMP52:%.*]] = alloca [[STRUCT_A:%.*]], align 8
; CHECK-NEXT: [[AGG_TMP52:%.*]] = alloca [[STRUCT_A]], align 8
; CHECK-NEXT: [[AGG_TMP1:%.*]] = alloca [[STRUCT_A]], align 8
; CHECK-NEXT: [[AGG_TMP:%.*]] = alloca [[STRUCT_A]], align 8
; CHECK-NEXT: [[AGG_TMP5:%.*]] = alloca [[STRUCT_A]], align 8
; CHECK-NEXT: br label [[TAILRECURSE:%.*]]
; CHECK: tailrecurse:
; CHECK-NEXT: [[I_TR:%.*]] = phi i32 [ [[I:%.*]], [[ENTRY:%.*]] ], [ [[ADD:%.*]], [[IF_END:%.*]] ]
; CHECK-NEXT: [[I_TR:%.*]] = phi i32 [ [[I]], [[ENTRY:%.*]] ], [ [[ADD:%.*]], [[IF_END:%.*]] ]
; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[I_TR]], 10
; CHECK-NEXT: br i1 [[CMP]], label [[RETURN:%.*]], label [[IF_END]]
; CHECK: if.end:
; CHECK-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds [[STRUCT_A]], ptr [[A:%.*]], i64 0, i32 0, i64 5
; CHECK-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds [[STRUCT_A]], ptr [[A]], i64 0, i32 0, i64 5
; CHECK-NEXT: [[TMP0:%.*]] = load i64, ptr [[ARRAYIDX]], align 8
; CHECK-NEXT: [[INC:%.*]] = add nsw i64 [[TMP0]], 1
; CHECK-NEXT: store i64 [[INC]], ptr [[ARRAYIDX]], align 8
; CHECK-NEXT: [[ARRAYIDX4:%.*]] = getelementptr inbounds [[STRUCT_A]], ptr [[B:%.*]], i64 0, i32 0, i64 5
; CHECK-NEXT: [[ARRAYIDX4:%.*]] = getelementptr inbounds [[STRUCT_A]], ptr [[B]], i64 0, i32 0, i64 5
; CHECK-NEXT: [[TMP1:%.*]] = load i64, ptr [[ARRAYIDX4]], align 8
; CHECK-NEXT: [[CALL:%.*]] = tail call i32 (ptr, ...) @printf(ptr nonnull dereferenceable(1) @.str, i64 [[INC]], i64 [[TMP1]])
; CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 80, ptr nonnull [[AGG_TMP]])
Expand Down Expand Up @@ -101,13 +102,13 @@ declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture) #2

; Function Attrs: noinline norecurse nounwind optnone uwtable
define dso_local i32 @main() local_unnamed_addr #3 {
; CHECK-LABEL: @main(
; CHECK-LABEL: define {{[^@]+}}@main() local_unnamed_addr {
; CHECK-NEXT: entry:
; CHECK-NEXT: [[AGG_TMP:%.*]] = alloca [[STRUCT_A:%.*]], align 8
; CHECK-NEXT: [[AGG_TMP1:%.*]] = alloca [[STRUCT_A]], align 8
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr align 8 [[AGG_TMP]], ptr align 8 @global, i64 80, i1 false)
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr align 8 [[AGG_TMP1]], ptr align 8 @global, i64 80, i1 false)
; CHECK-NEXT: tail call void @_Z7dostuff1AS_i(ptr byval(%struct.A) align 8 [[AGG_TMP]], ptr byval(%struct.A) align 8 [[AGG_TMP1]], i32 0)
; CHECK-NEXT: tail call void @_Z7dostuff1AS_i(ptr byval([[STRUCT_A]]) align 8 [[AGG_TMP]], ptr byval([[STRUCT_A]]) align 8 [[AGG_TMP1]], i32 0)
; CHECK-NEXT: ret i32 0
;
entry:
Expand Down
13 changes: 7 additions & 6 deletions llvm/test/Transforms/TailCallElim/tre-byval-parameter.ll
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature
; RUN: opt < %s -passes=tailcallelim -verify-dom-info -S | FileCheck %s

; the test was generated from the following C++ source:
Expand All @@ -24,21 +24,22 @@

; Function Attrs: uwtable
define dso_local i32 @_Z3fooi1S(i32 %count, ptr nocapture readonly byval(%struct.S) align 8 %p1) local_unnamed_addr #0 {
; CHECK-LABEL: @_Z3fooi1S(
; CHECK-LABEL: define {{[^@]+}}@_Z3fooi1S
; CHECK-SAME: (i32 [[COUNT:%.*]], ptr nocapture byval([[STRUCT_S:%.*]]) align 8 [[P1:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
; CHECK-NEXT: entry:
; CHECK-NEXT: [[AGG_TMP_I1:%.*]] = alloca [[STRUCT_S:%.*]], align 8
; CHECK-NEXT: [[AGG_TMP_I1:%.*]] = alloca [[STRUCT_S]], align 8
; CHECK-NEXT: [[AGG_TMP_I:%.*]] = alloca [[STRUCT_S]], align 8
; CHECK-NEXT: [[AGG_TMP14:%.*]] = alloca [[STRUCT_S]], align 8
; CHECK-NEXT: [[AGG_TMP:%.*]] = alloca [[STRUCT_S]], align 8
; CHECK-NEXT: [[AGG_TMP1:%.*]] = alloca [[STRUCT_S]], align 8
; CHECK-NEXT: br label [[TAILRECURSE:%.*]]
; CHECK: tailrecurse:
; CHECK-NEXT: [[COUNT_TR:%.*]] = phi i32 [ [[COUNT:%.*]], [[ENTRY:%.*]] ], [ [[ADD:%.*]], [[IF_END:%.*]] ]
; CHECK-NEXT: [[COUNT_TR:%.*]] = phi i32 [ [[COUNT]], [[ENTRY:%.*]] ], [ [[ADD:%.*]], [[IF_END:%.*]] ]
; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i32 [[COUNT_TR]], 10
; CHECK-NEXT: br i1 [[CMP]], label [[IF_THEN:%.*]], label [[IF_END]]
; CHECK: if.then:
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr nonnull align 8 dereferenceable(20) [[AGG_TMP]], ptr nonnull align 8 dereferenceable(20) [[P1:%.*]], i64 20, i1 false)
; CHECK-NEXT: [[CALL:%.*]] = tail call i32 @_Z3zoo1S(ptr nonnull byval(%struct.S) align 8 [[AGG_TMP]])
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr nonnull align 8 dereferenceable(20) [[AGG_TMP]], ptr nonnull align 8 dereferenceable(20) [[P1]], i64 20, i1 false)
; CHECK-NEXT: [[CALL:%.*]] = tail call i32 @_Z3zoo1S(ptr nonnull byval([[STRUCT_S]]) align 8 [[AGG_TMP]])
; CHECK-NEXT: br label [[RETURN:%.*]]
; CHECK: if.end:
; CHECK-NEXT: [[ADD]] = add nsw i32 [[COUNT_TR]], 1
Expand Down