This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC][GFX9] Corrected VOP3P relevant code to fix disassembler failures
ClosedPublic

Authored by dp on Jun 19 2017, 12:02 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

dp created this revision.Jun 19 2017, 12:02 PM

I've commented out a lot of asserts for packed constants. Should they be removed?

After quick glance -- You can run our standard set of AMDGPU backend tests (notably, CodeGen tests) *and* set of negative TestGen-produced tests to see if those are important.

I've commented out a lot of asserts for packed constants. Should they be removed?

After quick glance -- You can run our standard set of AMDGPU backend tests (notably, CodeGen tests) *and* set of negative TestGen-produced tests to see if those are important.

Of course, this is acceptable as a short-term solution only. I believe those asserts were introduced intentionally.

dp added a comment.Jun 19 2017, 12:43 PM

I've commented out a lot of asserts for packed constants. Should they be removed?

After quick glance -- You can run our standard set of AMDGPU backend tests (notably, CodeGen tests) *and* set of negative TestGen-produced tests to see if those are important.

I did run those tests and found no regressions.

dp added a comment.Jun 19 2017, 12:46 PM

I've commented out a lot of asserts for packed constants. Should they be removed?

After quick glance -- You can run our standard set of AMDGPU backend tests (notably, CodeGen tests) *and* set of negative TestGen-produced tests to see if those are important.

Of course, this is acceptable as a short-term solution only. I believe those asserts were introduced intentionally.

Most asserts look obsoleted. They assumed that packed constants are duplicated in both low and high 16 bits, but looks like they are not.

In D34360#784460, @dp wrote:

Most asserts look obsoleted. They assumed that packed constants are duplicated in both low and high 16 bits, but looks like they are not.

Basically yes, but I am suspecting that changes in SIMCCodeEmitter.cpp may affect CodeGen (e.g. may allow for wrong code instead of early failure in debug builds). Shall we ask Matt for feedback.

dp added a comment.Jun 19 2017, 2:08 PM
In D34360#784460, @dp wrote:

Most asserts look obsoleted. They assumed that packed constants are duplicated in both low and high 16 bits, but looks like they are not.

Basically yes, but I am suspecting that changes in SIMCCodeEmitter.cpp may affect CodeGen (e.g. may allow for wrong code instead of early failure in debug builds). Shall we ask Matt for feedback.

Sure.
Matt, your comments would be appreciated.

arsenm added inline comments.Jun 19 2017, 3:23 PM
lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp
252–261 ↗(On Diff #103089)

The problem is these operand types don't really exist. The inline immediates are actually encoded as 16-bit inline immediates and then VOP3P instructions are responsible for encoding the proper swizzle. This is leftover from when I thought they were encoded as the splat vector, so these should be removed

dp added inline comments.Jun 19 2017, 4:07 PM
lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp
252–261 ↗(On Diff #103089)

Do you mind if I submit this fix as is with V2 operand type issues postponed? We'll have stable code with more regression tests for future V2 fixing/refactoring.

arsenm added inline comments.Jun 19 2017, 4:55 PM
lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp
252–261 ↗(On Diff #103089)

Removing the asserts would be better than commenting them out, but yes

SamWot accepted this revision.Jun 20 2017, 2:58 AM

I agree, it's better to remove assert ,rather them commenting them out.
Otherwise, LGTM

This revision is now accepted and ready to land.Jun 20 2017, 2:58 AM
This revision was automatically updated to reflect the committed changes.