This is an archive of the discontinued LLVM Phabricator instance.

[MS] Emit S_HEAPALLOCSITE debug info
ClosedPublic

Authored by akhuang on Apr 16 2019, 3:19 PM.

Details

Summary

This emits labels around heapallocsite calls and S_HEAPALLOCSITE debug
info in codeview. Currently only changes FastISel, so emitting labels still
needs to be implemented in SelectionDAG.

Event Timeline

akhuang created this revision.Apr 16 2019, 3:19 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 16 2019, 3:19 PM
akhuang updated this revision to Diff 195475.Apr 16 2019, 3:21 PM

remove comment

akhuang updated this revision to Diff 195478.Apr 16 2019, 3:45 PM

more typos

akhuang updated this revision to Diff 195480.Apr 16 2019, 3:58 PM

Fix test case

hans added a comment.Apr 17 2019, 9:59 AM

I don't really know about the functionality here, just adding a few comments on the code itself.

clang/lib/CodeGen/CGDebugInfo.cpp
1973

I don't really know this code, but does this also work for void pointers, i.e. the if statement in the old code was unnecessary?

clang/test/CodeGen/debug-info-codeview-heapallocsite.c
19 ↗(On Diff #195480)

Is it not set with your new code though?

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1078

Is the cast necessary? Couldn't the tuple member be made a DIType* in the first place?

llvm/test/CodeGen/X86/label-heapallocsite.ll
2

Does llc have a "-fast-isel" flag or similar that could be used instead, to make it more clear that it's fast-isel that's significant for the test?

17

I guess the ModuleID and source_filename are unnecessary, so I'd dro pthem.

52

Both sets of attributes seem unnecessary so could probably be removed.

akhuang updated this revision to Diff 195783.Apr 18 2019, 10:43 AM
akhuang marked 4 inline comments as done.

Removed extraneous information from test; changed type to DIType

akhuang added inline comments.Apr 18 2019, 10:43 AM
clang/lib/CodeGen/CGDebugInfo.cpp
1973

I think getOrCreateType returns null if it gets a void type, so it doesn't quite work for void pointers. In theory we shouldn't be getting void pointers here since the type should be cast to something but that hasn't been implemented yet.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1078

Changed the tuple member to be a DIType.

rnk added inline comments.Apr 18 2019, 1:13 PM
clang/lib/CodeGen/CGDebugInfo.cpp
1973

I think in practice void will be pretty common, so I think we want to leave this as it is, and come up with some other workaround in CodeViewDebug.cpp. We can leave this as is and have the CodeViewDebug.cpp code interpret an empty tuple as a void type.

In hindsight, I think this "void is null" convention wasn't such a good idea, since we can't pass it through APIs like Instruction::setMetadata.

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1239

I think you could make addCodeViewHeapAllocSite take an MDNode* and then do this cast inside, and if the cast fails, see if it's the special empty tuple that means void, and put a null DIType* in the pointer.

akhuang updated this revision to Diff 195818.Apr 18 2019, 2:52 PM
akhuang marked 2 inline comments as done.
  • Pass void metadata as null DIType
akhuang updated this revision to Diff 195821.Apr 18 2019, 3:08 PM
akhuang marked 2 inline comments as done.
  • Changed test case back to original
akhuang marked an inline comment as done.Apr 18 2019, 3:10 PM
akhuang added inline comments.
llvm/test/CodeGen/X86/label-heapallocsite.ll
2

I couldn't find a flag that makes llc use fast-isel; it should soon work for both cases though.

rnk accepted this revision.Apr 18 2019, 3:22 PM

lgtm with a minor whitespace issue

clang/lib/CodeGen/CGDebugInfo.cpp
1972

Please revert the white space only change to this file.

llvm/lib/CodeGen/MachineFunction.cpp
816

I guess it's reasonable to treat any non-DIType as "void". The only other reasonable thing to do would be to report an error, but it's not worth it.

llvm/test/CodeGen/X86/label-heapallocsite.ll
2

FWIW, -O0 is the typical way to enable fast isel in other codegen tests, so even if it's opaque, it's consistent.

This revision is now accepted and ready to land.Apr 18 2019, 3:22 PM
akhuang updated this revision to Diff 195877.Apr 19 2019, 8:34 AM

whitespace fix

This revision was automatically updated to reflect the committed changes.