This is an archive of the discontinued LLVM Phabricator instance.

[DirectX] Add MC Register and Frame stubs
ClosedPublic

Authored by beanz on Jun 6 2022, 1:40 PM.

Details

Summary

This patch adds no-op stubs overrides for the MCRegisterInfo and
MCFrameLowering for the DirectX/DXIL code generation path.

Since DXIL will not generate MCInstrs these stubs do nothing, but they
need to exist so that the MC layer can be used to emit DXContainer
objects.

Diff Detail

Event Timeline

beanz created this revision.Jun 6 2022, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 1:40 PM
beanz requested review of this revision.Jun 6 2022, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 1:40 PM
beanz updated this revision to Diff 434596.Jun 6 2022, 1:42 PM

Forgot the CMake lines to generate the tablegen headers.

beanz updated this revision to Diff 434598.Jun 6 2022, 1:47 PM

Last update (I swear). Adding in the InstrInfo stub too because it is equivalently silly.

python3kgae added inline comments.Jun 6 2022, 2:08 PM
llvm/lib/Target/DirectX/MCTargetDesc/DirectXMCTargetDesc.h
18

DXIL registers?

beanz updated this revision to Diff 434609.Jun 6 2022, 2:11 PM

Fixing copy-paste comment errors.

beanz added inline comments.Jun 6 2022, 2:12 PM
llvm/lib/Target/DirectX/MCTargetDesc/DirectXMCTargetDesc.h
18

Doh... clearly some copypasta here...

Do we need any test for this change?

beanz added a comment.Jun 6 2022, 2:25 PM

Do we need any test for this change?

This is complicated... This particular patch is effectively NFC. Since there is no implementation to any of this code, it doesn't actually _do_anything (and really never will).

There are other patches in this stack (some not yet posted) which do have functionality, but they also aren't very testable because the MC-layer (and particularly the object streamer infrastructure) isn't architected to be as flexible for testing as other parts of LLVM.

Before any of this code becomes live a lot of other code all needs to fall into place. I'm working on getting the full patch set posted.

kuhar added inline comments.Jun 6 2022, 7:49 PM
llvm/lib/Target/DirectX/DirectXFrameLowering.h
30–35

I'm afraid these can lead to unused function argument warnings. Can we remove argument names to silence such warnings?

llvm/lib/Target/DirectX/DirectXInstrInfo.h
27

AFAIK at least one virtual function should be defined in a C++ file: https://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers

llvm/lib/Target/DirectX/DirectXRegisterInfo.h
25

Also here

beanz added inline comments.Jun 7 2022, 7:34 AM
llvm/lib/Target/DirectX/DirectXFrameLowering.h
30–35

I'm happy to make this change (I prefer it myself), but I do want to point out that LLVM is nowhere near clean for -Wunused-parameter, and it isn't a warning we generally support.

beanz updated this revision to Diff 434814.Jun 7 2022, 7:43 AM

Updates based on PR feedback.

kuhar accepted this revision.Jun 7 2022, 8:31 AM

LGTM but please get a second approval before submitting.

llvm/lib/Target/DirectX/DirectXFrameLowering.h
30–35

Ack, so it must have been llvm headers being pulled into other projects that generated such warnings for me in the past. Feels free to disregard this in other reviews that I sent as well.

This revision is now accepted and ready to land.Jun 7 2022, 8:31 AM

Looping in a few extra reviewers for some more eyes.

This looks fine to me. I don't see how this can be tested and if it never will have functionality in the future, there's not point in even trying.

arsenm accepted this revision.Jun 9 2022, 6:56 PM
MaskRay accepted this revision.Jun 11 2022, 12:19 AM
MaskRay added inline comments.
llvm/lib/Target/DirectX/DXILStubs.td
8

unneeded wrap

10

delete blank line

This revision was landed with ongoing or failed builds.Jun 17 2022, 7:16 PM
This revision was automatically updated to reflect the committed changes.