Don't ignore memcpy in GC root placement #23564
Conversation
|
#23522 might only be reproducible with |
|
Can you check this PR locally and see if it fixes your issue? |
|
Checking. |
src/llvm-late-gc-lowering.cpp
Outdated
| @@ -727,8 +727,8 @@ State LateLowerGCFrame::LocalScan(Function &F) { | |||
| for (auto it = BB.rbegin(); it != BB.rend(); ++it) { | |||
| Instruction &I = *it; | |||
| if (CallInst *CI = dyn_cast<CallInst>(&I)) { | |||
| if (isa<IntrinsicInst>(CI)) { | |||
| // Intrinsics are never GC uses/defs | |||
| if (isa<IntrinsicInst>(CI) && !isa<MemIntrinsic>(CI)) { | |||
There was a problem hiding this comment.
I assume this covers other intrinsics that touches memory? (like scatter, gather etc.)?
There was a problem hiding this comment.
And maybe add this condition back below where we check pointer_from_objref and memcmp so that they won't be treated as safepoints?
There was a problem hiding this comment.
Ah, I forgot about scatter and gather. There's probably also some machine specific intrinsic intrinsics that take memory operands. Probably easiest to just check debug and lifetime here (which are the only ones we emit frequently), but still declare all intrinsics as non-safepoint.
|
Just for fun, the probability that this didn't fix #23522 is smaller than 10^-200 ;-p |
| II->getIntrinsicID() == Intrinsic::lifetime_start || | ||
| II->getIntrinsicID() == Intrinsic::lifetime_end) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This version certainly looks better than the original one, which already looks good to me but you did remind me of a llvm intrinsic that I happened to notice the other day. llvm.load.relative is the only instruction I've found so far that returns a derived pointer. Probably don't need to worry about it as long as LLVM optimization passes don't emit it?
There was a problem hiding this comment.
Thanks, I didn't know about that intrinsic (in my defensive it's only about a year old ;)), but yeah, I don't think we have to worry about it.
If @yuyichao's analysis is correct, should fix #23522, though I've been unable to reproduce the failure myself (perhaps due to differing LLVM versions).