JumpThreading error removes `call void @bar()` instruction.

[dianqk]

I tried the reduced IR that from rust-lang/rust#115385:

declare void @set_value(ptr)

declare void @bar()

define void @foo(i1 %0) {
start:
  %v = alloca i64, align 8
  call void @set_value(ptr %v)
  %l1 = load i64, ptr %v, align 8, !range !0
  br i1 %0, label %bb0, label %bb2

bb0:                                              ; preds = %start
  %c1 = icmp eq i64 %l1, 0
  br i1 %c1, label %bb1, label %bb2

bb1:                                              ; preds = %bb0
  store i64 0, ptr %v, align 8
  br label %bb2

bb2:                                              ; preds = %bb1, %bb0, %start
  %l2 = load i64, ptr %v, align 8
  %1 = icmp eq i64 %l2, 2
  br i1 %1, label %bb3, label %bb4

bb3:                                              ; preds = %bb2
  call void @bar()
  ret void

bb4:                                              ; preds = %bb2
  ret void
}

!0 = !{i64 0, i64 2}

JumpThreading error removes call void @bar() instruction.

https://llvm.godbolt.org/z/ncv4rErdx

LLVM version is 17.0.0-rc3.

[efriedma-quic]

We changed the meaning of those metadata nodes precisely to make this transform legal... the current text in LangRef reflects this. The hoisted load returns poison, and its value is never used if it would be poison.

It's possible we missed something when we made that change, but I suspect the issue you're seeing is unrelated.

[efriedma-quic]

See https://reviews.llvm.org/D141386

[jdoerfert]

I think the reproducer simply got lost. The godbold link seems to show a valid transformation. The range metadata is fine, as it can be violated w/o causing an issue.
Alive2 is happy with it as well: https://alive2.llvm.org/ce/z/oJ6kp-
(I replaced %c with false and %v with poison).

In the original reproducer, the load with the range feeds into a branch (which is UB on poison), causing the issue, I suppose:

Maybe we need to reduce again to find the location that miscompiles this.

[dianqk]

@efriedma-quic

See https://reviews.llvm.org/D141386

Yes, I have read this before this issue. Hoisting a poison is safe only when it doesn't introduce more UB.

@jdoerfert

Maybe we need to reduce again to find the location that miscompiles this.

I got the reduced IR, the branch on poison is not this end-to-end reason. I put this on the top description.

[nikic]

As far as I can tell, JumpThreading is the faulty transform here. My guess to what happens is this: JumpThreading performs load CSE under some circumstances. It correctly calls combineMetadataForCSE in that case -- as you can see, the !range metadata gets correctly dropped from the transformation result. However, the load will still be cached in LVI, even though the cached value is no longer valid for the new use. Possibly all that is missing here is an LVI invalidation when adjusting the metadata on the load?

[dianqk]

I seem to get it. I can't infer the range of the next load from the range of the previous load, even if there is no store instruction in between.
But what about the br instruction? I should have understood hoist br to be converted to select, preventing the poison from propagating. Thanks again.
I'm preparing a new PR.

[dianqk]

Reopen it for the backport.