Page MenuHomePhabricator

[AMDGPU] Fix use of HasModifiers in VopProfile
ClosedPublic

Authored by mbrkusanin on Jan 15 2021, 9:13 AM.

Details

Summary

HasModifiers should be true if at least one modifier is used.
This should make the use of this field bit more consistent.

Diff Detail

Event Timeline

mbrkusanin created this revision.Jan 15 2021, 9:13 AM
mbrkusanin requested review of this revision.Jan 15 2021, 9:13 AM

I would prefer to include HasClamp inside "field bit HasModifiers =" but VOPProfile allows clamp to be explicitly enabled through "bit _EnableClamp = 0" which still messes up the logic a little bit (like in class getIns64). Not sure if there is some other reason to keeping clamp separate from other modifiers.

This is why there are still a few "let HasClamp = 1;" lines. Ideally we would only want "= 0" for special cases where some modifiers are not used.

"HasModifiers should not be true if at least one modifier is used."

Do you mean "*should* be true"?

mbrkusanin edited the summary of this revision. (Show Details)Jan 15 2021, 9:22 AM

"HasModifiers should not be true if at least one modifier is used."

Do you mean "*should* be true"?

Yes. Thanks.

arsenm accepted this revision.Jan 15 2021, 10:20 AM

Do we really need HasModifiers if breaking this down to src modifiers and omod?

This revision is now accepted and ready to land.Jan 15 2021, 10:20 AM
Joe_Nash requested changes to this revision.Jan 19 2021, 11:47 AM

I'm getting failing tests in check-llvm-mc-disassembler-amdgpu when applying this patch, can you take a look at that please?

This revision now requires changes to proceed.Jan 19 2021, 11:47 AM
  • Undid few changes to fix disassemble tests.

Do we really need HasModifiers if breaking this down to src modifiers and omod?

This
"field bit HasSrc0Mods = HasModifiers;"
basically tells us that we can replace all uses of HasModifiers with HasSrc0Mods. I guess it's more of a question of readability.

Joe_Nash accepted this revision.Jan 22 2021, 12:02 PM
Joe_Nash added inline comments.
llvm/lib/Target/AMDGPU/SIInstrInfo.td
2071

It would be even more consistent if HasClamp was here too, so that HasModifiers is true if HasClamp is true. However, I think that can be a separate patch.

This revision is now accepted and ready to land.Jan 22 2021, 12:02 PM
This revision was automatically updated to reflect the committed changes.