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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 30765 Build 30764: arc lint + arc unit
Event Timeline
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. |
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. |
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. |
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. |
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. |
Please revert the white space only change to this file.