This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Fix SILoadStoreOptimizer when writes cannot be merged due register dependencies
ClosedPublic

Authored by nhaehnle on Oct 20 2016, 8:41 AM.

Details

Summary

When finding a match for a merge and collecting the instructions that must
be moved, keep in mind that the instruction we merge might actually use one
of the defs that are being moved.

Fixes piglit spec/arb_enhanced_layouts/execution/component-layout/vs-tcs-load-output[-indirect].

The fact that the ds_read in the test case is not eliminated suggests that
there might be another problem related to alias analysis, but that's a
separate problem: this pass should still work correctly even when earlier
optimization passes missed something or were disabled.

Event Timeline

nhaehnle updated this revision to Diff 75308.Oct 20 2016, 8:41 AM
nhaehnle retitled this revision from to AMDGPU: Fix SILoadStoreOptimizer when writes cannot be merged due register dependencies.
nhaehnle updated this object.
nhaehnle added reviewers: arsenm, tstellarAMD.
nhaehnle added a subscriber: llvm-commits.
lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
259

What does f(w) mean in this comment? Also, before this patch was this pass merging these two writes?

nhaehnle added inline comments.Oct 21 2016, 1:50 AM
lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
259

It's supposed to mean some function of w :)

Before this patch, the writes were merged and compilation failed later because "use not dominated by defs".

tstellarAMD accepted this revision.Oct 21 2016, 6:18 AM
tstellarAMD edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Oct 21 2016, 6:18 AM
arsenm edited edge metadata.Oct 24 2016, 4:55 AM

Should this be using MachineInstr::isSafeToMove instead of this custom logic?

Should this be using MachineInstr::isSafeToMove instead of this custom logic?

After looking at the implementation this isn't right. The comment for it seems misleading to me since it doesn't take an iterator for where you're moving it, and only checks a few things on the single instruction

This revision was automatically updated to reflect the committed changes.