This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fix double space in disassembly of ds_gws_sema_* with gds
ClosedPublic

Authored by foad on Oct 28 2020, 6:58 AM.

Details

Summary

By setting up the AsmStrings correctly we can remove some special cases
from AMDGPUInstPrinter::printOffset.

Diff Detail

Event Timeline

foad created this revision.Oct 28 2020, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2020, 6:58 AM
foad requested review of this revision.Oct 28 2020, 6:58 AM
foad added a comment.Oct 28 2020, 7:00 AM

Unfortunately I can't reliably test this change with FileCheck. It seems to tolerate a double space in the input when I only want to match a single space, even if I use a regexp pattern like {{\ }}.

arsenm added inline comments.Oct 28 2020, 7:00 AM
llvm/lib/Target/AMDGPU/DSInstructions.td
333

Why is there a $gds operand here if it's not used for printing?

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
142

Single quotes

Unfortunately I can't reliably test this change with FileCheck. It seems to tolerate a double space in the input when I only want to match a single space, even if I use a regexp pattern like {{\ }}.

There's a -strict-whitespace flag

foad updated this revision to Diff 301286.Oct 28 2020, 7:55 AM

Rebase after adding -strict-whitespace to the tests.

foad updated this revision to Diff 301288.Oct 28 2020, 8:10 AM

Use single quotes.

foad marked an inline comment as done.Oct 28 2020, 8:11 AM
foad added inline comments.
rampitec added inline comments.Oct 28 2020, 11:42 AM
llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
142

Are you sure it didn't add an extra space in some other instructions? Are there any other instructions where offset can be the first operand?

foad added inline comments.Oct 28 2020, 2:53 PM
llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
142

Are you sure it didn't add an extra space in some other instructions?

Well it didn't break any disassembler tests with -strict-whitespace. I did wonder if it might add an extra space at the *end* of some other instruction, that would be hidden because -show-encoding pads with spaces to a fixed column number before adding the "; encoding:" comment. But...

Are there any other instructions where offset can be the first operand?

No, I have looked through the output of tablegen and I can't find any others.

rampitec accepted this revision.Oct 28 2020, 2:54 PM
This revision is now accepted and ready to land.Oct 28 2020, 2:54 PM
foad added inline comments.Oct 28 2020, 2:57 PM
llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
142

An alternative fix would be to remove this special handling of OpNo==0 and change the AsmStrings to remove the space between the mnemonic and the $offset operand. I can try that out.

rampitec added inline comments.Oct 28 2020, 2:58 PM
llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
142

If it works it would be cleaner.

foad updated this revision to Diff 301543.Oct 29 2020, 2:22 AM

Cleaner fix.

foad edited the summary of this revision. (Show Details)Oct 29 2020, 2:22 AM
foad added inline comments.Oct 29 2020, 3:23 AM
llvm/lib/Target/AMDGPU/DSInstructions.td
333
rampitec accepted this revision.Oct 29 2020, 10:14 AM

Thanks, Jay!

This revision was landed with ongoing or failed builds.Oct 29 2020, 10:32 AM
This revision was automatically updated to reflect the committed changes.