This allows us to discharge many pointer comparisons based on byval arguments.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
2674–2679 | In theory, yes. I'll rewrite to assume either alloca or argument for now. |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
2571 | Can't this function be simplified to isByValArgOrAllocaOrGlobalVar(V1) && isByValArgOrAllocaOrGlobalVar(V2)? Is there some reason your implementation is going out of the way to not handle the GlobalVariable + GlobalVariable case? (Note that GlobalVariable != GlobalObject, so this does not included aliases). |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
2571 | The bit of code your commenting on was the original structure - I moved it, but didn't change it. When I posted this, I hadn't dug into the global piece in detail. I have now. Global are slightly tricky because not all them are guaranteed disjoint - or at least that's what the code in constant folding seems to imply? We don't actually get here with globals - they're constantexprs - but I didn't want to risk adding rarely executed potentially divergent behavior. I think that your suggestion probably is okay, but I'd strongly prefer to go with the current structure for now, and restructure in a separate change for risk isolation. My hope is to eventually pull this out in a way where we can reuse it in constant folding as well. |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
2571 | Hm yeah, I guess the two global case is not legal for unnamed_addr globals, which may be merged. I think it may still be worthwhile to write this in a way that explicitly excludes that case, rather than listing different permutations of possible combinations. Something like this, maybe? auto IsDisjointObject = [](Value *V) { auto *Arg = dyn_cast<Argument>(V); return isa<AllocaInst>(V) || isa<GlobalVariable>(V) || (Arg && Arg->hasByValAttr()); }; if (!IsDisjointObject(V1) || !IsDisjointObject(V2)) return false; // Two globals may have the same address. // TODO: Make this specific to unnamed_addr globals. return !isa<GlobalVariable>(V1) || !isa<GlobalVariable>(V2); |
LGTM
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
2571 | But I think it's also okay as-is... |
Since you're in the area, have you seen https://github.com/llvm/llvm-project/issues/45725 ?
I had not. Skimmed it, doesn't seem directly related. I'm not even sure there's a bug there, much less one immediately relevant here.
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
2571 | I will play with some options here and try posting something in another review. |
I wrote
https://github.com/llvm/llvm-project/issues/54615
about a crash that started happening with this patch.
Can't this function be simplified to isByValArgOrAllocaOrGlobalVar(V1) && isByValArgOrAllocaOrGlobalVar(V2)? Is there some reason your implementation is going out of the way to not handle the GlobalVariable + GlobalVariable case? (Note that GlobalVariable != GlobalObject, so this does not included aliases).