This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Fix off-by-one in SIRegisterInfo::eliminateFrameIndex
ClosedPublic

Authored by nhaehnle on Dec 15 2015, 3:11 PM.

Details

Summary

The method insertNOPs expected the number of wait states to be passed as
parameter, while eliminateFrameIndex passed the immediate argument for the
S_NOP, leading to an off-by-one error. Rename the method to make the
meaning of its parameter clearer. The number of 4 / 5 wait states (which
is what the method has always _tried_ to do according to the comment) is
correct according to the hardware docs.

I stumbled upon this while trying to track down the cause of
https://bugs.freedesktop.org/show_bug.cgi?id=93264. While clearly needed,
this patch unfortunately does not fix that bug...

Diff Detail

Event Timeline

nhaehnle updated this revision to Diff 42923.Dec 15 2015, 3:11 PM
nhaehnle retitled this revision from to AMDGPU: Fix off-by-one in SIRegisterInfo::eliminateFrameIndex.
nhaehnle updated this object.
nhaehnle added reviewers: arsenm, tstellarAMD.
nhaehnle added a subscriber: llvm-commits.
lib/Target/AMDGPU/SIInstrInfo.h
444–445

To me this is still confusing. I would probably go a step further and re-name the function insertWaitStates().

Otherwise, LGTM.

nhaehnle updated this revision to Diff 43135.Dec 17 2015, 7:23 AM

Address review comment

tstellarAMD accepted this revision.Dec 17 2015, 7:58 AM
tstellarAMD edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Dec 17 2015, 7:58 AM
This revision was automatically updated to reflect the committed changes.