Improve the codegen of type symbols for debugging#27613
Improve the codegen of type symbols for debugging#27613jabraham17 merged 44 commits intochapel-lang:mainfrom
Conversation
1d58dcc to
1355edb
Compare
9754603 to
c62b401
Compare
…h of other type cleanups Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
…ugging Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
c62b401 to
7524ca8
Compare
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
DanilaFe
left a comment
There was a problem hiding this comment.
Looks good, though I didn't study the details of the seds in the test files, or sanity-check the dwarfdump output.
| if (constant->init) { | ||
| fprintf(outfile, " = %s", constant->init->codegen().c.c_str()); | ||
| } | ||
| fprintf(outfile, "%s = %s", sym->codegen().c.c_str(), var->codegen().c.c_str()); |
There was a problem hiding this comment.
Double-checking: this changes our generated C code, but you're saying it's acceptable because, even supposing an enum like:
// (Chapel)
enum { a, b = 12, c, d = 5 }Other transformations convert the above into:
// (Chapel)
enum { a, b = 12, c = 13, d = 5 }Which was equivalent to the generated C code:
// C++
enum { a, b = 12, c = 13, d = 5 }Here, C will start number at zero by default, so the new form
enum { a = 0, b = 12, c = 13, d = 5 }Retains the original behavior?
There was a problem hiding this comment.
Yes. Put another way: I took the logic we had for generating enums with LLVM (which emulates what the C backend does) and used it for both LLVM and C
There was a problem hiding this comment.
And if it helps, we have tests like test/types/enum/enumCast.chpl which lock in the integer value based on C semantics, and they still pass with this change
| INT_FATAL("Unable to find LLVM type for field %s of type %s in struct %s", | ||
| field->cname, fts->cname, type->symbol->name); | ||
| } | ||
| return nullptr; |
There was a problem hiding this comment.
Because now I have added the INT_FATAL if they are missing. Its an internal error instead of just a comment in the code. Which means if a user hits this in the field (unlikely, if both fts->getLLVMType(); and getTypeLLVM(fts->cname) fail, there are probably other issues beyond debug info), they get an internal error and report it, and we fix it
There was a problem hiding this comment.
And while debugging, I have yet to actually hit this branch
| myTypeDescriptors[type] = N; | ||
| return llvm::cast_or_null<llvm::DIType>(N); | ||
| } | ||
| else if(this_class->aggregateTag == AGGREGATE_CLASS) { | ||
| N = dibuilder->createStructType( | ||
| get_module_scope(defModule), | ||
| name, | ||
| get_file(defModule, defFile), | ||
| defLine, | ||
| layout.getTypeSizeInBits(ty), | ||
| 8*layout.getABITypeAlign(ty).value(), | ||
| llvm::DINode::FlagZero, | ||
| derivedFrom, | ||
| dibuilder->getOrCreateArray(EltTys)); |
There was a problem hiding this comment.
are my eyes playing tricks on me, or was this all the same code?
There was a problem hiding this comment.
This was pretty much all the same code. There are some very very subtle differences that I preserved as best I can. Basically the differences stemmed from "pointer to class" vs "class"
| CHPL_COMM!=none | ||
| COMPOPTS <= --no-local |
There was a problem hiding this comment.
Why not skipif these instead of suppressing?
There was a problem hiding this comment.
Because ideally, one day they work. These are copied from the existing targetVar.suppressif
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
|
With a quick glance at the OP, this feels more like a feature freeze item than a code freeze item…? |
|
This was discussed last week in the release round table as a code freeze item. Perhaps it wasn't fully clear the extent of the changes |
Fixes a few errors from merging #27613 - fixes merge issues between #27613 and #27777 in the pretty printer - adds missing skipifs and suppressifs where needed - while there, removes an unrelated .bad file which is causing other nightly failures - fixes the typed pointers build with LLVM 14 [Reviewed by @lydia-duncan]
Improves the codegen of chapel type symbols, especially for debugging.
Key Improvements
llvmDebug.cppto make way for future improvementsThis PR also makes a few improvements to
sub_testMakes progress on #27603
Future work
xandyare garbage, but only ifRhas no other fields and an explicitinit[Reviewed by @DanilaFe ]