[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

Wed Jan 6 08:35:02 PST 2021

dblaikie added a comment.

In D91734#2482280 <https://reviews.llvm.org/D91734#2482280>, @probinson wrote:

> In D91734#2480930 <https://reviews.llvm.org/D91734#2480930>, @dblaikie wrote:
>
>> I haven't looked closely, but I take it this one test modification seems reasonable to you? I guess we're stepping into some subexpressions on a function call that we previously didn't? (they didn't have any instructions attributed to them until now, but it's not unreasonable that they have them attributed - for materializing constants to pass to a function call when the function call/constants are split over multiple lines, etc)
>
> Exactly.  In each case, the test is trying to "next" over a function call; gcc attributes all parameter evaluation to the function name, while clang will attribute each parameter to its own location.  And when the parameters span multiple source lines, the is_stmt heuristic kicks in, so we stop on each line with actual parameters.
>
> This is not ideal IMO; I'd rather be more principled about (a) stop at every parameter, or (b) stop at no parameters.  But without reworking how we do is_stmt, I think fiddling the test to do enough single-steps to actually get past the function call is okay.

I get that it's a bit unreliable for the user - though GCC's approach isn't so different. If any of the parameter evaluations are themselves function calls, I think it does still multistep as well.

Actually, it seems it does not. Somewhere back around GCC 6 it did what we're doing, but since about 8 at least, no matter what the expressions (at least I tested for + and function call) it attributes all the instructions to the function call line - even the backtrace is misleading - I don't think it's better, just different.

But, yes, we could possibly do better with more strategic is_stmt, but that's a big/complex piece of work to tackle.

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

https://reviews.llvm.org/D91734