[PATCH] D115060: [clang-format][NFC] Code Tidies in UnwrappedLineFormatter

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:215
+    const auto &NextLine = *I[1];
+    const auto *PreviousLine = I != AnnotatedLines.begin() ? I[-1] : nullptr;
+    if (NextLine.Type == LT_Invalid || NextLine.First->MustBreakBefore)
----------------
owenpan wrote:
> HazardyKnusperkeks wrote:
> > owenpan wrote:
> > > HazardyKnusperkeks wrote:
> > > > owenpan wrote:
> > > > > I would move this line to just before handling empty record blocks below.
> > > > I'd rather keep the definitions close together.
> > > If it were just a simple initialization, it wouldn't matter much. However, it would be a bit wasteful as `PreviousLine` always gets computed here even if the function may return before the pointer would get chance to be used. If you really want to keep all related definitions together, wouldn't you want to move lines 214-215 up to right after line 211?
> > In a release build I'm betting that the compiler is smart enough to never calculate `PreviousLine` and that performance doesn't really matter was shown in D116318.
> > 
> > But yeah moving it up to `TheLine` is consistent and will do.
> > In a release build I'm betting that the compiler is smart enough to never calculate `PreviousLine`
> 
> No compiler will (or should) skip generating code that calculates `PreviousLine`, which may or may not get used at runtime. I'm not aware of any compiler that is smart enough to move the initialization code to just before where the variable is first used.
> 
> To illustrate the difference, I compiled the code below with `clang++ -Wall -std=c++11 -O3 -S foo.cpp`:
> ```
> #include <vector>
> 
> using namespace std;
> using Type = vector<int *>;
> 
> int f(const Type &V, Type::const_iterator I) {
>   const auto *P = I != V.begin() ? I[-1] : nullptr;
>   if (I == V.end())
>     return 0;
>   return *P;
> }
> ```
> The generated code in foo.s includes the following:
> ```
> 	.cfi_startproc
> ; %bb.0:
> 	ldr	x8, [x0]
> 	cmp	x8, x1
> 	b.eq	LBB0_3
> ; %bb.1:
> 	ldur	x8, [x1, #-8]
> 	ldr	x9, [x0, #8]
> 	cmp	x9, x1
> 	b.eq	LBB0_4
> LBB0_2:
> 	ldr	w0, [x8]
> 	ret
> LBB0_3:
> 	mov	x8, #0
> 	ldr	x9, [x0, #8]
> 	cmp	x9, x1
> 	b.ne	LBB0_2
> LBB0_4:
> 	mov	w0, #0
> 	ret
> 	.cfi_endproc
> ```
> As you can see, the initialization code was neither optimized away nor moved.
> 
> When I changed the function body to:
> ```
>   if (I == V.end())
>     return 0;
>   const auto *P = I != V.begin() ? I[-1] : nullptr;
>   return *P;
> ```
> The generated code became much simpler:
> ```
> 	.cfi_startproc
> ; %bb.0:
> 	ldr	x8, [x0, #8]
> 	cmp	x8, x1
> 	b.eq	LBB0_2
> ; %bb.1:
> 	ldur	x8, [x1, #-8]
> 	ldr	w0, [x8]
> 	ret
> LBB0_2:
> 	mov	w0, #0
> 	ret
> 	.cfi_endproc
> ```
> and that performance doesn't really matter was shown in D116318.

It wasn't quite the same there as we must pay either in the caller or in the callee. That being said, I agree that the performance hit here would be negligible.


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

https://reviews.llvm.org/D115060