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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram marked 2 inline comments as done.
tmsriram added inline comments.


================
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;
+; }
----------------
dblaikie wrote:
> tmsriram wrote:
> > dblaikie wrote:
> > > 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?)
> > @amharc 
> > 
> > Thanks for taking a look, I will change the code.
> > 
> > The FDEs cannot be fewer, one is needed per section.  This is because FDEs do not support ranges like debug info and it is only low-high PC.  In theory, each section could be in arbitrary non-contiguous locations and hence conservatively we need four.
> > 
> > We have a CFI dedup that will reduce the size of each FDE and move the common parts out to the CIE.  I split that out of this patch to keep this simple.
> > 
> > 
> > > Could it be fewer, what about "void f1(); void f2(bool b) { if (b) f1(); }" is that still adequate to test this  functionality?)
> > 
> > This will not capture the deduping well because we need atleast 4 basic blocks to have duplicates.  The entry and the exit block have other CFI instructions.
> > 
> > 
> > We have a CFI dedup that will reduce the size of each FDE and move the common parts out to the CIE. I split that out of this patch to keep this simple.
> 
> Then I'd generally suggest keeping this test simple too - though I can understand the benefit of having the diff on the test just demonstrate how things got better in the later change (rather than introducing test changes or new tests that demonstrate the new behavior). Your call.
Sure,  Iwill make it simple and evolve it when submitting the dedup patch.


================
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;
----------------
dblaikie wrote:
> 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)




It was done so that more callee-saved registers are used and when more callee saved registers are used cfi_offset directives are needed for it.  The .s looks like this for a basic block that does the computation:

_Z7computebiiiiiiiiiiii.1:              # %if.then
	.cfi_startproc
	.cfi_def_cfa %rbp, 16
	.cfi_offset %rbx, -48
	.cfi_offset %r12, -40
	.cfi_offset %r14, -32
	.cfi_offset %r15, -24
	.cfi_offset %rbp, -16

Each basic block that goes in a different section must emit cfi directives for callee-saved registers.  The parameters is to make sure the caller saved registers are taken and the callee saved registers are forced so that we can check that the cfi emission indeed works for callee saved registers. 



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

https://reviews.llvm.org/D79978