Optimization for types implementing drop seems broken on LLVM 19 #125373
Comments
rustbot
added
the
needs-triage
durin42
added
A-LLVM
|
FYI @jwong101 |
|
@rustbot claim |
jieyouxu
added
the
T-compiler
|
After taking a look at this, the problem seems to be that GVN isn't propagating the pointer equalities in the following IR after this PR: define void @src(ptr noundef %buf, ptr noundef %start, ptr noundef %end) {
bb2:
%iter.exhausted = icmp eq ptr %end, %start
br i1 %iter.exhausted, label %bb5, label %bb3
bb3: ; preds = %bb2
%iter.empty.loop = icmp eq ptr %end, %buf
br i1 %iter.empty.loop, label %bb5, label %exit
bb5: ; preds = %bb2, %bb3
%self.end = phi ptr [ %buf, %bb3 ], [ %start, %bb2 ]
;%self.end = phi ptr [ %end %bb3 ], [ %end %bb2 ] <- gvn used to propogate this
%_710.i.i = icmp eq ptr %end, %self.end
br i1 %_710.i.i, label %exit, label %bb6
bb6: ; preds = %bb5
%inner.cap = load i64, ptr %self.end, align 8, !noundef !0
call void @effect(i64 %inner.cap)
br label %exit
exit: ; preds = %bb3, %bb5, %bb6
ret void
}
define void @tgt(ptr noundef %buf, ptr noundef %start, ptr noundef %end) {
ret void
}Before, GVN would blindly propagate However, it should be sound to propagate pointer equality to just comparisons, which the PR does when the only uses are Unfortunately, this doesn't help in our case, since the phi node is also used by a load. My naive idea would be to either create duplicate phi nodes that are only used by There's probably a better way, as I'm not that familiar with LLVM's GVNpass. Is there another approach I could take for solving the above problem; should I be looking at another pass? |
|
@jwong101 That test case is a bit over-reduced. In the original test, we actually do know that the underlying objects are the same, but we have to look through phi nodes to detect this. Something like this fixes the failure: diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 1d54a66705a2..13bd07312d8e 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -743,7 +743,11 @@ static bool isPointerAlwaysReplaceable(const Value *From, const Value *To,
if (isa<Constant>(To) &&
isDereferenceablePointer(To, Type::getInt8Ty(To->getContext()), DL))
return true;
- if (getUnderlyingObject(From) == getUnderlyingObject(To))
+ SmallVector<const Value *> FromObjs, ToObjs;
+ getUnderlyingObjects(From, FromObjs);
+ getUnderlyingObjects(To, ToObjs);
+ if (FromObjs.size() == 1 && ToObjs.size() == 1 &&
+ FromObjs.back() == ToObjs.back())
return true;
return false;
}We wouldn't want to do literally that, but something along those lines. I'll try to put up a patch soon. |
|
Finally got around to this, here is the patch: llvm/llvm-project#99509 |
|
@rustbot label +llvm-fixed-upstream |
rustbot
added
the
llvm-fixed-upstream
|
Closing this, as it is fixed and we already have a codegen test. |
This was only recently fixed by #123878 (bug was #120493), but the test fails on LLVM 19 like this:
Filing this so we don't lose track of it.
The text was updated successfully, but these errors were encountered: