This is an archive of the discontinued LLVM Phabricator instance.

[Disassember][NFCI] Use strong type for instruction decoder
ClosedPublic

Authored by maksfb on Mar 22 2022, 11:13 AM.

Details

Summary

All LLVM backends use MCDisassembler as a base class for their
instruction decoders. Use "const MCDisassembler *" for the decoder
instead of "const void *". Remove unnecessary static casts.

Diff Detail

Event Timeline

maksfb created this revision.Mar 22 2022, 11:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 11:13 AM
maksfb requested review of this revision.Mar 22 2022, 11:13 AM
foad added a comment.Mar 23 2022, 2:30 AM

Remove unnecessary static casts.

Can you explain which ones are removed? In the AMDGPU backend it looks like they are all still required, to cast from MCDisassembler * to AMDGPUDisassembler *.

No objection to the patch btw.

Can you explain which ones are removed? In the AMDGPU backend it looks like they are all still required, to cast from MCDisassembler * to AMDGPUDisassembler *.

Right, I removed casts where no further code changes were required. E.g., if the pointer was used to call an AMDGPUDisassembler-specific function, we still need to cast. It's possible I've missed some cases. Please let me know.

skan added a comment.Mar 23 2022, 7:17 PM

Can you explain which ones are removed? In the AMDGPU backend it looks like they are all still required, to cast from MCDisassembler * to AMDGPUDisassembler *.

Right, I removed casts where no further code changes were required. E.g., if the pointer was used to call an AMDGPUDisassembler-specific function, we still need to cast. It's possible I've missed some cases. Please let me know.

I think most of the removed casts are const void *->const MCDisassembler * b/c we use MCDisassembler as parameters explicitly in this patch , which seems reasonable to me. From the logs, I believe FixedLenDecoderEmitter.cpp defined the function prototype first, and then all the targets had to follow it.

skan added a comment.Mar 23 2022, 7:40 PM

Added @grosbach as a reviewer, who defined the interfaces of decodeToMCInst and decodeInstruction in https://reviews.llvm.org/rGecaef49f59bb4e414f95d89b7c6b35eb9adad314 in 2012.
After ten years, I believe we don't need such generic interface now.

LGTM generally.

skan accepted this revision.Mar 24 2022, 6:17 PM
This revision is now accepted and ready to land.Mar 24 2022, 6:17 PM
This revision was landed with ongoing or failed builds.Mar 25 2022, 6:54 PM
This revision was automatically updated to reflect the committed changes.