tailcallelim introduces write to readonly byval parameter

[erikdesjardins]

With the following IR:

define void @foo(ptr readonly byval(i64) %x) {
start:
  %new_x = alloca i64, align 8
  store i64 0, ptr %new_x, align 8
  call void @foo(ptr %new_x)
  ret void
}

opt -passes="tailcallelim" results in (https://godbolt.org/z/9arGs5xhx):

define void @foo(ptr readonly byval(i64) %x) {
  %new_x1 = alloca i64, align 1
  %new_x = alloca i64, align 8
  br label %tailrecurse

tailrecurse:                                      ; preds = %tailrecurse, %start
  store i64 0, ptr %new_x, align 8
  call void @llvm.memcpy.p0.p0.i64(ptr align 1 %new_x1, ptr align 1 %new_x, i64 8, i1 false)
  call void @llvm.memcpy.p0.p0.i64(ptr align 1 %x, ptr align 1 %new_x1, i64 8, i1 false)
  br label %tailrecurse
}

declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias nocapture readonly, i64, i1 immarg)

%x is now written to, but it still has its readonly attribute.

LangRef seems to imply that this is illegal:

The copy is considered to belong to the caller not the callee (for example, readonly functions should not write to byval parameters).

However, Alive2 is fine with writes to readonly byval arguments (but does flag identical IR without byval): https://alive2.llvm.org/ce/z/T6yxU-

---

This leads to an end-to-end miscompile, where the following recursion (always terminating after one call):

define void @foo(ptr noalias byval(i64) %x) {
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 @foo(ptr %new_x)
  br label %end

end:
  ret void
}

is converted to an infinite loop with opt -O3 (https://godbolt.org/z/9zbn6hno9):

define void @foo(ptr noalias nocapture readonly byval(i64) %x) local_unnamed_addr #0 {
  %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:                                          ; preds = %start, %recurse
  br label %recurse

end:                                              ; preds = %start
  ret void
}

---

If LangRef is right, the bug is in tailcallelim. If Alive2 is right, the bug is in whatever other transforms assume that readonly is meaningful on byval arguments.

Upstream issue: rust-lang/rust#114312

[dianqk]

If Alive2 is correct, it is possible that LICM caused this error.
https://llvm.godbolt.org/z/sxTrYYq6q.

[dianqk]

If Alive2 is correct, it is possible that LICM caused this error. https://llvm.godbolt.org/z/sxTrYYq6q.

Bisected: 01859da.
Maybe we should remove noalias/readonly in tailcallelim?
cc @pcwalton @nikic

[nikic]

Yes, I think we should be dropping readonly in tailcallelim.

Generally, changing function attributes during a (non-IPO) transform is very fishy, in particular because the attribute might have been propagated out to callers and used there.

However, the situation with byval + readonly is quite special, as the note in LangRef indicates. From the perspective of a caller, byval is basically always "readonly", in that the memory passed to the argument will always be copied to the byval stack slot and never modified. The readonly attribute in this case only describes the internal memory effects of the function, not the effects visible to the caller. As such, I believe that dropping readonly during the transform would be safe in this particular case.

[dianqk]

I try to submit a patch that removes readonly.
https://reviews.llvm.org/D156793

[dianqk]

Reopen and wait for the post CI to look ok then pick to 17.