Page MenuHomePhabricator

AMDGPU: Fix memory dependence analysis by considering the offset.
ClosedPublic

Authored by cfang on Feb 15 2019, 11:09 AM.

Details

Summary

In SIInstrInfo::areMemAccessesTriviallyDisjoint, the current implementation ignored the offset when creates MemoryLocation
for alias analysis, and thus may report wrong result when offset is not zero. This patch fixes the issue.

Diff Detail

Repository
rL LLVM

Event Timeline

cfang created this revision.Feb 15 2019, 11:09 AM
arsenm requested changes to this revision.Feb 15 2019, 11:16 AM
arsenm added inline comments.
lib/Target/AMDGPU/SIInstrInfo.cpp
2218–2232 ↗(On Diff #187050)

Why can't you use MI.mayAlias and delete this?

This revision now requires changes to proceed.Feb 15 2019, 11:16 AM
cfang marked an inline comment as done.Feb 15 2019, 1:54 PM
cfang added inline comments.
lib/Target/AMDGPU/SIInstrInfo.cpp
2218–2232 ↗(On Diff #187050)

This intends to fix a bug in an application. We can use MI.mayAlias and delete areMemAccessesTriviallyDisjoint.
However, we need to make sure areMemAccessesTriviallyDisjoint is indeed useless because things developed there
are not for on reason. We need extensive tests.
So I propose to add this patch first to fix the bug. Then we can spend time to investigate the way to safely remove the stuff in
areMemAccessesTriviallyDisjoint and replace it with MI.mayAlias.

cfang updated this revision to Diff 187272.Feb 18 2019, 2:09 PM

Use MachineInstr::mayAlias to replace areMemAccessesTriviallyDisjoint in LoadStoreOptimizer pass.

cfang updated this revision to Diff 187273.Feb 18 2019, 2:32 PM

Remove the unused Target Instruction Info argument in a couple functions.

arsenm accepted this revision.Feb 18 2019, 2:33 PM

LGTM

test/CodeGen/AMDGPU/ds-combine-with-dependence.ll
69 ↗(On Diff #187272)

Typo oroginal

This revision is now accepted and ready to land.Feb 18 2019, 2:33 PM
arsenm added inline comments.Feb 18 2019, 2:34 PM
lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
264–265 ↗(On Diff #187273)

This merged part of the other patch

cfang marked an inline comment as done.Feb 18 2019, 2:44 PM
cfang added inline comments.
lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
264–265 ↗(On Diff #187273)

What do you mean here? I think this is the staring point of the current patch. We are going to replace
TII->areMemAccessesTriviallyDisjoint with MI.mayAlias to fix the bug.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2019, 3:01 PM