This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Fix getInstSizeInBytes
ClosedPublic

Authored by nhaehnle on Aug 13 2018, 3:31 AM.

Details

Summary

Add some optional code to validate getInstSizeInBytes for emitted
instructions. This flushed out some issues which are fixed by this
patch:

  • Streamline getInstSizeInBytes
  • Properly define the VI readlane/writelane instruction as VOP3
  • Fix the inline constant determination. Specifically, this change fixes an issue where a 32-bit value of 0xffffffff was recorded as unsigned. This is equal to -1 when restricting to a 32-bit comparison, and an inline constant can be used.

Change-Id: Id87c3b7975839da0de8156a124b0ce98c5fb47f2

Diff Detail

Event Timeline

nhaehnle created this revision.Aug 13 2018, 3:31 AM
This revision is now accepted and ready to land.Aug 13 2018, 7:24 AM
arsenm added inline comments.Aug 13 2018, 7:57 AM
lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
320

I strongly dislike assertion checks inside debug printing

320

Why can't you just assert after the instruction is emitted normally?

nhaehnle updated this revision to Diff 161948.Aug 22 2018, 6:58 AM

Always do the sanity check in debug builds.

I was originally worried about how expensive this is, but it
doesn't show up in the running time of all the tests in
test/CodeGen/AMDGPU.

arsenm added inline comments.Aug 22 2018, 9:58 AM
lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
320

You could always put this under an EXPENSIVE_TEST checks or whatever it's called, then at least the bots will be testing it

Always do the sanity check in debug builds.

I was originally worried about how expensive this is, but it
doesn't show up in the running time of all the tests in
test/CodeGen/AMDGPU.

These are by design almost all small, so that makes sense. It might not be representative in the real world

arsenm added inline comments.Aug 22 2018, 10:33 AM
lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
321–327

We don't actually have a generic CPU? We just pick a default?

Always do the sanity check in debug builds.

I was originally worried about how expensive this is, but it
doesn't show up in the running time of all the tests in
test/CodeGen/AMDGPU.

These are by design almost all small, so that makes sense. It might not be representative in the real world

We shouldn't be running debug builds in the real world anyway.

lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
321–327

No, we don't. That's precisely the problem I mentioned elsewhere at some point and I brought up the possibility of just deciding that running without specifying a CPU is unsupported.

If you don't specify a CPU (or the CPU is unknown), you get a set of inconsistent feature bits that doesn't correspond to any real CPU. The consequence is that all sorts of MachineInstrs are generated during CodeGen which aren't actually considered supported by MC, so entering the instruction encoding logic will fail.

I briefly looked into ways of fixing this, and it's of course be possible, but it'd require fixing a *lot* of the existing tests.

arsenm added inline comments.Aug 23 2018, 12:44 AM
lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
321–327

It looks like I changed this in r310258. I thought this was still defaulting to kaveri for HSA and tahiti for anything else

nhaehnle added inline comments.Aug 23 2018, 1:10 AM
lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
321–327

We end up generating FLAT_LOAD_DWORD_ci without having FeatureCIInsts, for example.

Always do the sanity check in debug builds.

I was originally worried about how expensive this is, but it
doesn't show up in the running time of all the tests in
test/CodeGen/AMDGPU.

These are by design almost all small, so that makes sense. It might not be representative in the real world

We shouldn't be running debug builds in the real world anyway.

This will run with fast debug builds, which is what I usually use

nhaehnle updated this revision to Diff 162823.Aug 28 2018, 3:52 AM

Guard sanity check with EXPENSIVE_CHECKS

arsenm accepted this revision.Aug 28 2018, 5:11 AM

LGTM

This revision was automatically updated to reflect the committed changes.