<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 6, 2018 at 8:26 AM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">Ryan Prichard via Phabricator via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> writes:<br>
<br>
<br>
</span>> Index: lib/MC/MCObjectStreamer.cpp<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- lib/MC/MCObjectStreamer.cpp<br>
> +++ lib/MC/MCObjectStreamer.cpp<br>
> @@ -51,17 +51,36 @@<br>
> PendingLabels.clear();<br>
> }<br>
><br>
> +static Optional<uint64_t> absoluteSymbolDiff(const MCSymbol *Hi,<br>
> + const MCSymbol *Lo) {<br>
> + if (!Hi->getFragment() || Hi->getFragment() != Lo->getFragment() ||<br>
> + Hi->isVariable() || Lo->isVariable())<br>
> + return None;<br>
> +<br>
> + return Hi->getOffset() - Lo->getOffset();<br>
> +}<br>
<br>
Add a comment saying this is just an optimization to avoid creating a<br>
fragment when the final value is known.<br></blockquote><div><br></div><div>I'll add a comment above <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">absoluteSymbolDiff.</span></div><div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></div><div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">I don't think the optimization prevents the creation of new fragments. For an absolute (Hi - Lo) expression, the general code path calls <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">MCObjectStreamer::EmitValueImpl, then <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">MCObjectStreamer::EmitBytes. EmitValueImpl and EmitBytes do a lot of the same work. With the optimization, EmitValueImpl is skipped, and <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">EmitBytes is called more directly. I think the important part is that it avoids allocating and evaluating an MCExpr tree.</span></span></span></span></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> void MCObjectStreamer::<wbr>emitAbsoluteSymbolDiff(const MCSymbol *Hi,<br>
> const MCSymbol *Lo,<br>
> unsigned Size) {<br>
> - // If not assigned to the same (valid) fragment, fallback.<br>
> - if (!Hi->getFragment() || Hi->getFragment() != Lo->getFragment() ||<br>
> - Hi->isVariable() || Lo->isVariable()) {<br>
> + Optional<uint64_t> Diff = absoluteSymbolDiff(Hi, Lo);<br>
> + if (!Diff.hasValue()) {<br>
> MCStreamer::<wbr>emitAbsoluteSymbolDiff(Hi, Lo, Size);<br>
> return;<br>
> }<br>
><br>
> - EmitIntValue(Hi->getOffset() - Lo->getOffset(), Size);<br>
> + EmitIntValue(*Diff, Size);<br>
<br>
<br>
I think you can write this as<br>
<br>
if (Optional<uint64_t> Diff = ...) {<br>
EmitIntValue(*Diff, Size);<br>
return;<br>
}<br>
MCStreamer::<wbr>emitAbsoluteSymbolDiff(Hi, Lo, Size);<br></blockquote><div><br></div><div>Done.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
This patch does two things:<br>
<br>
* Remove the manual accounting and replace it with using assembler<br>
expressions.<br>
* Use uleb.<br>
<br>
The first part should be an awesome nop cleanup. Could it be split out<br>
of this patch?<br></blockquote><div><br></div><div>Yeah, that should be possible.</div><div><br></div><div>Before my patch, LLVM outputs the TTBase offset for non-SJLJ mode even when the type table is empty. After my patch, the offset is omitted. I'll include that change in the second patch.</div><div><br></div><div>I assume each patch needs to pass check-llvm cleanly, so I'll need to fix up some of the same tests twice. It shouldn't be much trouble.</div><div><br></div><div>The first patch will still change how the type table is aligned. i.e. It will typically replace TTBase LEB padding with alignment padding before the type info table.</div><div><br></div><div>Thanks,<br></div><div>-Ryan</div><div><br></div></div></div></div>