[PATCH] D39418: [ELF] - Split processSectionCommands().

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 1 02:12:23 PDT 2017


grimar added a comment.

In https://reviews.llvm.org/D39418#912510, @ruiu wrote:

> Still not sure if this is an improvement. When I ask for splitting a loop, it doesn't ask for moving code from a loop to a new function, but actually split a loop. For example, assume this loop:
>
>   for (...) {
>     A;
>     B;
>   }
>   
>
> When you split the loop, you'll get this:
>
>   for (...)
>     A;
>   for (...)
>     B;
>   
>
> This is I think an improvement because it is now clear that A and B doesn't depend each other. B might depend on A, but it is still better than mutually-dependent code. But if you just move code outside of the loop like this:


I see what you mean. Issue here that A and B are actually dependent. This loop simplified representation is:

  for (...) {
   createSymbol();
   createSection();
  }

If I would split it to:

  for (...) {
   createSymbol();
  }
  for (...) {
   createSection();
  }

It would break absolute2.s which script is `SECTIONS { .text : { *(.text) } foo = ABSOLUTE(_start) + _start; };`
And error would be: unable to evaluate expression: input section .text has no output section assigned
because assignment tries to access symbol in .text section wich is not yet created.

If I would split it to reversed order:

  for (...) {
   createSection();
  }
  for (...) {
   createSymbol();
  }

Then it would break early-assign-symbol.s case which script is `SECTIONS { aaa = foo | 1; .text  : { *(.text*) } }`
And it checks that we report "{{.*}}.script:1: unable to evaluate expression: input section .text has no output section assigned" here.

These experiments does not take into account that we also create symbols inside sections, what might add additional issues
when trying to split this logic out. And generally I think such dependency is correct and does not feel like it worth splitting as it not simple.

What we can do here is to reduce the loop itself and that is what I tried to do in this patch.


https://reviews.llvm.org/D39418





More information about the llvm-commits mailing list