[PATCH] D82085: [TRE] markTails marks call sites as tailcalls though some of them are not.

[Alexey Lapshin via Phabricator via cfe-commits]

avl added a comment.

> It's OK to set "tail" on any call that satisfies these requirements (from https://llvm.org/docs/LangRef.html#call-instruction): "Both markers [tail and musttail] imply that the callee does not access allocas from the caller. The tail marker additionally implies that the callee does not access varargs from the caller."



> "tail" does not mean that the call *must* be generated as a tail call. It just means that it's safe to generate it as a tail call if it turns out to be possible (e.g. if the compiler can prove that @noarg doesn't return, or if it can prove that all the code after the call to @noarg has no effect, or so on).

Yes, that is understood: ""tail" does not mean that the call *must* be generated as a tail call".
I intended to make picture consistent: when markTails sees function body, it is evident that the first call is not a tail call. I agree that a compiler "can prove that @noarg doesn't return, or if it can prove that all the code after the call to @noarg has no effect" and this first call could become tail-call. Probably the suitable strategy, in this case, would be to recalculate marking when it is broken(instead of creating false positive marks). There are other cases(compiler could add function calls), which could break existing marking and then would be necessary to recalculate marking.

Having many false positive "tail" marks could create a confusing picture. 
I would describe the full problem which I am trying to solve.
(I assumed this patch would be the first one for that problem):

cat test.cpp

  int count;
  __attribute__((noinline)) void globalIncrement(const int* param) { count += *param; }
  
  void test(int recurseCount)
  {
      if (recurseCount == 0) return;
      {
        int temp = 10;
        globalIncrement(&temp);
      }
      test(recurseCount - 1);
  }

TRE is not done for that test case. There are two reasons for that: 
First is that AllocaDerivedValueTracker does not use the PointerMayBeCaptured interface, and it does not see that &temp is not escaped.
Second is that AllCallsAreTailCalls is used as a pre-requisite for making TRE.
So it requires that all calls would be marked as "tail". This looks too restricted.
Instead, it should check that "&temp" is not escaped in globalIncrement() and that "test" 
is a tail recursive call not using allocas. I think the confusion happened exactly because "tail" marking was done for all calls(not for the real tailcalls).

Thus I planned to do the following fixes:

1. cleanup "tail" marking.(this patch)
2. do not use "AllCallsAreTailCalls" as a pre-requisite for TRE.(this patch).
3. use PointerMayBeCaptured inside AllocaDerivedValueTracker.

What do you think about all of this?

It seems to me that it would be good to have consistent "tail" marking. 
But if it does not look like a good idea, I could continue to point 2. and 3.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82085