[PATCH] D79978: Call Frame Information (CFI) Handling for Basic Block Sections

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

(don't have any particular feedback on most of the patch - CFI stuff isn't something I'm especially familiar with - just a couple of optional thoughts on simplifying/clarifying the tests)



================
Comment at: llvm/test/DebugInfo/X86/basicblock-sections-cfi.ll:4-10
+; From:
+; int foo(int a) {
+;   if (a > 20)
+;     return 2;
+;   else
+;     return 0;
+; }
----------------
I /might/ find this simpler:

```
void f1();
void f2();
void f3(bool b) {
  if (b)
    f1();
  else
    f2();
}
```

But no big deal either way - maybe some more exposition on why 1 CIE and 4FDEs are expected? (I guess 1 CIE shares some common data across the 4 FDEs which are for the 4 basic blocks? Could it be fewer, what about "void f1(); void f2(bool b) { if (b)  f1(); }" is that still adequate to test this functionality?)


================
Comment at: llvm/test/DebugInfo/X86/basicblock-sections-cfiinstr.ll:23-30
+; int compute(bool k, int p1, int p2, int p3, int p4, int p5, int p6, int p7, int p8, int p9, int pa, int pb, int pc) {
+;  int result;
+;  if (k)
+;    result = p1 * p2 + p3 / p4 - p5 * p6 + p7 / p8 - p9 * pa + pb / pc;
+;  else
+;    result = p1 / p2 - p3 * p4 + p5 / p6 - p7 * p8 + p9 / pa - pb * pc;
+;  return result;
----------------
Seems like a surprisingly large amount of computation - is it there for a reason? needed to push some optimization or layout decisions? Could it all use the same operation (just all multiplication, for instance) or is the different operations significant? (Well, I guess they have to differ between the two branches - but could all be the same within each one?) does it need 12 parameters? Could it be fewer & use a function call?

(etc, etc - simple test case, maybe some comments describing what's significant about the features of it that are needed to demonstrate the desired behavior, etc)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79978/new/

https://reviews.llvm.org/D79978