[MemCpyOpt] Call slot optimization doesn't respect writeonly

[aeubanks]

$ cat /tmp/a.ll
declare void @g(ptr)

define void @f(ptr dead_on_unwind noalias writable writeonly sret(i64) align 8 %0) {
  %a = alloca i64
  call void @g(ptr %a)
  call void @llvm.memcpy(ptr align 8 %0, ptr align 8 %a, i64 8, i1 false)
  ret void
}
$ opt -p memcpyopt -S /tmp/a.ll
declare void @g(ptr)

define void @f(ptr dead_on_unwind noalias writable writeonly sret(i64) align 8 %0) {
  %a = alloca i64, align 8
  call void @g(ptr %0)
  ret void
}

If @g is not writeonly in regards to the ptr param, we may end up reading from %0 in @g which is marked writeonly.

[nikic]

I think this is another instance of our writeonly spec being untenable. writeonly should return poison when being read, in which case the transform is fine (modulo undef vs poison differences, but that's a general problem).

[aeubanks]

Makes sense. Is there any reason not to make that change to the LangRef now?

[nikic]

Makes sense. Is there any reason not to make that change to the LangRef now?

Don't think so.

[aeubanks]

Actually even with that LangRef change, we still would get the following miscompile:

$ cat /tmp/a.ll
declare void @use(i64)

define void @j(ptr %p) optnone noinline {
  %i = load i64, ptr %p
  call void @use(i64 %i)
  ret void
}

define void @h(ptr %p) {
  store i64 3, ptr %p
  call void @j(ptr %p)
  ret void
}

define void @g(ptr dead_on_unwind noalias writable dereferenceable(8) align 8 %p) minsize {
  %a = alloca i64
  call void @h(ptr %a)
  call void @llvm.memcpy(ptr align 8 %p, ptr align 8 %a, i64 8, i1 false)
  ret void
}

define void @f(ptr dead_on_unwind noalias %p) {
  call void @g(ptr %p)
  store i64 43, ptr %p
  ret void
}

# -O3 also works
$ opt -p 'cgscc(function-attrs,inline,function(memcpyopt,dse))' -S /tmp/b.ll -use-noalias-intrinsic-during-inlining=0
declare void @use(i64)

; Function Attrs: noinline optnone
define void @j(ptr %p) #0 {
  %i = load i64, ptr %p, align 4
  call void @use(i64 %i)
  ret void
}

define void @h(ptr %p) {
  store i64 3, ptr %p, align 4
  call void @j(ptr %p)
  ret void
}

; Function Attrs: minsize
define void @g(ptr dead_on_unwind noalias nocapture writable writeonly align 8 dereferenceable(8) %p) #1 {
  %a = alloca i64, align 8
  call void @h(ptr %p)
  ret void
}

define void @f(ptr dead_on_unwind noalias nocapture nonnull writeonly %p) {
  call void @j(ptr writeonly %p)
  store i64 43, ptr %p, align 4
  ret void
}

; Function Attrs: nocallback nofree nounwind willreturn memory(argmem: readwrite)
declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias nocapture readonly, i64, i1 immarg) #2

attributes #0 = { noinline optnone }
attributes #1 = { minsize }
attributes #2 = { nocallback nofree nounwind willreturn memory(argmem: readwrite) }

notice the missing store in f before the call to j that was initially present in h. this is caused by inferring writeonly on g's param, performing call slot optimization on g's param/alloca, propagating the writeonly on the g param to the call to j when inlining g -> f and h -> f, and finally DSEing the store because the argument we pass from f to j is writeonly

we started seeing this miscompile after #89024, although ultimately call slot optimization is at fault because we may read from the call slotted pointer after writing to it (like in h/j). I think we basically need to intersect the attributes between the pointer param and the alloca onto the pointer param.

@goldsteinn

[efriedma-quic]

I think there's something a little more fundamental here... beyond the one throwaway sentence in the definition of the writeonly attribute, we don't really have a notion of "only-writable" memory in LLVM IR. The C++ memory model assumes that "writing" memory may also involve reading it.

Along those lines, marking a function "memory(write)" doesn't really mean the function doesn't read any memory; it just implies that the behavior of the function doesn't depend on outside state. Nothing outside the function can tell the difference, anyway.

Given that, I think we should redefine writeonly to specifically tie it to the function's entry point: if you try to read a value that was written before entry to the function, it's poison. But you can read values written by the function itself. I suspect this is pretty easy to implement: we just need to make function-attrs more conservative about propagating writeonly. (At least, with a quick search, I can't find anywhere else that cares about writeonly on a function definition; most places only care about writeonly on call instructions.)

[nikic]

My general understanding of how writeonly and memory(write) should work basically matches what @efriedma-quic said. These describe memory effects, and as such it only matters what is externally observable, not what happens inside the function.

A caveat here is that there is a certain asymmetry between readonly and writeonly. We can't say that "reading a value and writing back the same value" satisfies "readonly", because doing so could introduce a data race or a fault. And for readonly, we do make use of it inside the function definition as well (for the readonly noalias case).

I went ahead and reverted 5f99a7a for now to fix the immediate miscompile, while we pin down the best solution here.

I'm generally not a fan of the kind of attribute clearing approach in #95179 -- I know we do this in various places already, but it's fragile because we can only clear attributes locally, while they may have already been propagated upwards.