This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Improve Aliasing of operations to static alloca
ClosedPublic

Authored by niravd on May 18 2017, 6:38 PM.

Details

Summary

Memory accesses offset from frame indices may alias, e.g., we may merge write from function arguments passed on the stack when they are contiguous. As a result, when checking aliasing, we consider the underlying frame index's offset from the stack pointer.

Static allocs are realized as stack objects in SelectionDAG, but its offset is not set until post-DAG causing DAGCombiner's alias check to consider access to static allocas to frequently alias. Modify isAlias to consider access between static allocas and access from other frame objects to be considered aliasing.

Many test changes are included here. Most are fixes for tests which indirectly relied on our aliasing ability and needed to be modified to preserve their original intent.

The remaining tests have minor improvements due to relaxed ordering. The exception is CodeGen/X86/2011-10-19-widen_vselect.ll which has a minor degradation dispite though the pre-legalized DAG is improved.

Event Timeline

niravd created this revision.May 18 2017, 6:38 PM
rnk added inline comments.May 19 2017, 3:06 PM
lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
160 ↗(On Diff #99516)

I don't like setting SPOffset once and then setting it again later during PEI. Passes will get different results over time, and that seems bad.

Why does TCO cause frame index aliasing? That seems like a problem we might be able to address in TCO.

I'd much rather let alias analysis assume that loads and stores based on different non-fixed frame indices can't alias. Fixed frame indices can probably alias in things like va_arg handling, but in that case we should have a reliable fixed offset here. It should hold that fixed and un-fixed stack objects don't overlap.

These offsets here also don't account for alignment.

niravd updated this revision to Diff 104813.Jun 29 2017, 8:49 PM
niravd retitled this revision from [DAG] Mark SPOffset for static allocas attached to stack frame to [DAG] Improve Aliasing of operations to static alloca.
niravd edited the summary of this revision. (Show Details)

New patch

niravd marked an inline comment as done.Jun 29 2017, 8:56 PM
niravd added inline comments.
lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
160 ↗(On Diff #99516)

These concerns should all be resolved (explanation in new patch summary).

rnk added inline comments.Jun 30 2017, 10:40 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
16712–16713

I think we could strengthen this to !MFI.isFixedObjectIndex(A->getIndex()). Fixed objects have offsets, and all non-fixed objects will be allocated in non-overlapping memory during PEI regardless of whether they have allocas or not.

niravd updated this revision to Diff 104977.Jun 30 2017, 8:13 PM
niravd marked an inline comment as done.

Use IsFixedObjectIndex.

niravd added inline comments.Jun 30 2017, 8:20 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
16712–16713

Seems reasonable. Changing this allows some reordering of statepoint updates.

niravd updated this revision to Diff 105509.Jul 6 2017, 12:39 PM
niravd marked an inline comment as done.

rebase past extracted tests.

rnk accepted this revision.Jul 6 2017, 12:51 PM

lgtm

This revision is now accepted and ready to land.Jul 6 2017, 12:51 PM
This revision was automatically updated to reflect the committed changes.
MatzeB added a subscriber: MatzeB.Jul 10 2017, 1:41 PM

Hi, this change leads to llvm-test-suite tramp3d-v4 failing (at least on macOS systems).

Example: http://lab.llvm.org:8080/green/job/perf_darwin_x86_O3flto/11000/

This should reproduce the problem:

$ cd test-suite
$ mkdir build
$ cd build
$ cmake -DCMAKE_C_COMPILER=/Users/mbraun/dev/public_llvm/assertbuild/bin/clang -GNinja ..
$ ninja tramp3d-v4
$ llvm-lit -v MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4.test

I will revert this in the meantime.