Page MenuHomePhabricator

AMDGPU/SILoadStoreOptimizer: fix a likely bug introduced recently
ClosedPublic

Authored by nhaehnle on Oct 9 2019, 3:56 AM.

Details

Summary

We should check for same instruction class before checking whether they
have the same base address, else we might iterate out of bounds of a
MachineInstr operands list. The InstClass check is also cheaper.

This was introduced in SVN r373630.

Diff Detail

Event Timeline

nhaehnle created this revision.Oct 9 2019, 3:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 3:56 AM

Testcase?

I didn't really have the bandwidth to think deeply about how to trigger this. It's more of a drive-by observation.

Ping

On the issue of a test case: I don't think it's practically possible to hit the crash I was worried about right now, because AddrIdx[0] happens to be small enough in all cases that it will be a valid operand index in all cases, and then hasSameBaseAddress will bail out for machine instructions of a different class because the register comparison of the first index will fail. However, that's just an artifact of how machine operand indices happen to be laid out today. I still maintain that the change itself is the right way to do things.

arsenm accepted this revision.Nov 13 2019, 4:53 PM

LGTM

This revision is now accepted and ready to land.Nov 13 2019, 4:53 PM
This revision was automatically updated to reflect the committed changes.