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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram marked an inline comment as done and 2 inline comments as not done.
tmsriram added inline comments.


================
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;
----------------
tmsriram wrote:
> dblaikie wrote:
> > tmsriram wrote:
> > > dblaikie wrote:
> > > > tmsriram wrote:
> > > > > 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. 
> > > > > 
> > > > Ah, OK - a comment might be handy to describe that?
> > > > 
> > > > And rather than the somewhat arbitrary computation, perhaps an opaque function call would suffice? Or would that introduce other complications for spills/saves/etc?
> > > > 
> > > > Maybe using a pass by value struct as the parameter type so the long parameter list doesn't have to be repeated?
> > > Simplified the test and added comments.  Having more than 4 integers in the struct seems to go to the stack though the ABI says upto 32 bytes.
> > > 
> > Ah - the comment's good, thanks! Not sure about the code changes - I was hoping for more uniformity (& brevity as a second-order benefit), but having a struct, then 3 struct parameters and some ints lacks the uniformity I was hoping for.
> > 
> > Also the arithmetic looks sort of arbitrarily complicated (& raises the question, as a reader (for me at least), why is it complicated? Is the particular sequence of arithmetic important in some way?). Is a more uniform operation (like all addition) not viable due to vectorizing or something? (is a function call inadequate because of other spill issues (eg: void f1(bool k, int a, int b.... ) { int result; if (k) { result f2(a, b, ... ); } else { result = f3(a, b, ...); } return result; })?)
> @wmi who is a register allocation expert.   I am not an expert but we need to use callee saved registers in some manner.  Function calls would still only use caller-save registers and unless we have a *callee* where there is serious computation combined with a dearth of scratch registers (caller-save), callee saved registers will be avoided.  Does this make sense :) ? 
> 
> I can try to simplify the computation or spend some time to see if I can produce simpler code that can still use callee saved registers, but I dont have concrete ideas now.
> 
> 
Ah! I think you are right :), sorry about that!  It does use callee saved registers for function calls. Let me simplify this along these lines


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

https://reviews.llvm.org/D79978