This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] gfx1010: prefer V_MUL_LO_U32 over V_MUL_LO_I32
ClosedPublic

Authored by rampitec on May 3 2019, 10:57 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

rampitec created this revision.May 3 2019, 10:57 AM

What does the signedness of the multiply even mean?

What does the signedness of the multiply even mean?

It does not. There is problem with the instruction itself.

What does the signedness of the multiply even mean?

It does not. There is problem with the instruction itself.

I believe the goal is to generally use the u32 suffix for arithmetic instructions where the signedness doesn't matter, but the ISA has generally been inconsistent about that and the i32 vs. u32 confusion here is simply because they simultaneously (1) changed some encodings around and (2) changed the canonical mnemonic of the instruction. So the _i32 and _u32 variants are really the same instruction semantically.

(The sign does matter for mul_hi)

arsenm added a comment.May 4 2019, 5:47 AM

Can this just use the u32 version on all targets then?

Can this just use the u32 version on all targets then?

It's sp3 compatibility. I am bot sure u32 is also available in all ASICs.

Can this just use the u32 version on all targets then?

It's sp3 compatibility. I am bot sure u32 is also available in all ASICs.

OK, I have checked. All ASICs support u32 version. I will change it.

rampitec planned changes to this revision.May 6 2019, 9:51 AM
rampitec updated this revision to Diff 198321.EditedMay 6 2019, 12:08 PM
rampitec retitled this revision from AMDGPU] gfx1010: prefer V_MUL_LO_U32 over V_MUL_LO_I32 to [AMDGPU] gfx1010: prefer V_MUL_LO_U32 over V_MUL_LO_I32.
rampitec edited the summary of this revision. (Show Details)

Unconditionally use V_MUL_LO_U32.

kzhuravl accepted this revision.May 6 2019, 3:06 PM

LGTM

This revision is now accepted and ready to land.May 6 2019, 3:06 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2019, 3:24 PM