[PATCH] D57541: [DAGCombiner] Eliminate dead stores to stack.
Clement Courbet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 7 01:11:49 PST 2019
courbet added inline comments.
================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15386
+
+ // We walk up the chains to find stores.
+ SmallVector<SDValue, 8> Chains = {N->getOperand(0)};
----------------
niravd wrote:
> courbet wrote:
> > niravd wrote:
> > > I think we should merge this with FindBetterChains. At the least we should have load/store aliasing be able to bypass unrelated lifetime nodes and vice versa.
> > I'm not exactly sure how to merge it, but teaching it to bypass unrelated lifetime nodes sounds like a good idea. Done.
> I was thinking of this as two combines: One sharing logic with FindBetterChains to improve LIFETIME_END's chain to simplify it's chain to (presumably just the store chain) and the other to check the chain of a LIFETIME_END like we do with dead stores via overlapping stores. You already have the second.
>
>
>
I see. There is an issue with making `GatherAllAliases` work with `LifetimeSDNode`: `LifetimeSDNode` cannot be a `MemSDNode` (or `LSBaseSDNode`) because `MemSDNode`s really represent memory **accesses**, and lifetime nodes really are just references to memory, they never access it. Code manipulating the `MemSDNode` member `MachineMemOperand` assumes either load or store, and changing this would require significant changes.
On the other hand now that I've made `FindBetterChain` able to go past lifetime start/end nodes as you've suggested in previous comments, it will be able to move stores out of unrelated `LIFETIME_{START,END}`.
================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15397
+ break;
+ case ISD::LIFETIME_END:
+ Chains.push_back(Chain.getOperand(0));
----------------
niravd wrote:
> LIFETIME_START as well?
Indeed, but only for `lifetime_start`s that do not overlap. I'm not checking the overlap for lifetime_end because you cannot have:
```
lifetime_end(a);
lifetime_end(a);
```
but you might have:
```
lifetime_start(a);
lifetime_end(a);
lifetime_start(a); // 3
lifetime_end(a); // 4
```
and you don't want 4 to go past 3.
================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15402
+ StoreSDNode *ST = dyn_cast<StoreSDNode>(Chain);
+ if (ST->isVolatile() || !ST->hasOneUse() || ST->isIndexed())
+ continue;
----------------
niravd wrote:
> You can remove "!ST->hasOneUse()" now that you have the indexed check.
I still need to check the `!hasOneUse()` check because the store might be depended on by several users.
================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15412
+ LifetimeBaseIndex.dump(); dbgs() << "\n");
+ CombineTo(ST, ST->getChain());
+ return SDValue(N, 0);
----------------
niravd wrote:
> Nit: return CombineTo(ST, ST->getChain);
Not sure I understand why ?
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:90
+ int64_t PtrDiff;
+ if (BasePtr0.equalBaseIndex(BasePtr1, DAG, PtrDiff)) {
+ IsAlias = !((NumBytes0 <= PtrDiff) || (PtrDiff + NumBytes1 <= 0));
----------------
niravd wrote:
> This is "contains".
Not exactly. `contains` means "one is within the other" (and is not symmetrical), here it's "one might overlap the other" (and is symmetrical). I've added graphical comments + the symmetric formula for reference.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57541/new/
https://reviews.llvm.org/D57541
More information about the llvm-commits
mailing list