Page MenuHomePhabricator

[DAG] Teach BaseIndexOffset to correctly handle with indexed operations
ClosedPublic

Authored by niravd on Jan 3 2018, 7:46 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

I would prefer to have a regression test, but it's okay if you can't come up with a way to write one.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
90 ↗(On Diff #128521)

Does this need to be something like "BaseIndexOffset(Base, N->getOffset(), 0, false);"? Currently, it looks like this code is discarding the increment.

niravd updated this revision to Diff 128771.Jan 5 2018, 10:43 AM

Correct failing on non-constant indices.

I would prefer to have a regression test, but it's okay if you can't come up with a way to write one.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
90 ↗(On Diff #128521)

Good catch. This won't work on pre-decrement though as we'd need the negated index. I've changed it to mark such cases as returning no information and modifying other uses to check for a null base.

efriedma added inline comments.Jan 5 2018, 11:02 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13360 ↗(On Diff #128771)

Does this null check still work properly given that BaseIndexOffset::match can return a null base?

niravd added inline comments.Jan 5 2018, 11:38 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13360 ↗(On Diff #128771)

The indexed check @ line 13350 precludes the new case from happening here.

efriedma accepted this revision.Jan 5 2018, 11:46 AM

LGTM

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13360 ↗(On Diff #128771)

Oh, I see.

This revision is now accepted and ready to land.Jan 5 2018, 11:46 AM
This revision was automatically updated to reflect the committed changes.