[PATCH] D42720: [CodeGen] Switch non-SJLJ EH encoding to uleb128

[Ryan Prichard via llvm-commits]

On Tue, Feb 6, 2018 at 8:26 AM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> Ryan Prichard via Phabricator via llvm-commits
> <llvm-commits at lists.llvm.org> writes:
>
>
> > Index: lib/MC/MCObjectStreamer.cpp
> > ===================================================================
> > --- lib/MC/MCObjectStreamer.cpp
> > +++ lib/MC/MCObjectStreamer.cpp
> > @@ -51,17 +51,36 @@
> >    PendingLabels.clear();
> >  }
> >
> > +static Optional<uint64_t> absoluteSymbolDiff(const MCSymbol *Hi,
> > +                                             const MCSymbol *Lo) {
> > +  if (!Hi->getFragment() || Hi->getFragment() != Lo->getFragment() ||
> > +      Hi->isVariable() || Lo->isVariable())
> > +    return None;
> > +
> > +  return Hi->getOffset() - Lo->getOffset();
> > +}
>
> Add a comment saying this is just an optimization to avoid creating a
> fragment when the final value is known.
>

I'll add a comment above absoluteSymbolDiff.

I don't think the optimization prevents the creation of new fragments. For
an absolute (Hi - Lo) expression, the general code path calls
MCObjectStreamer::EmitValueImpl,
then MCObjectStreamer::EmitBytes. EmitValueImpl and EmitBytes do a lot of
the same work. With the optimization, EmitValueImpl is skipped, and EmitBytes
is called more directly. I think the important part is that it avoids
allocating and evaluating an MCExpr tree.

>  void MCObjectStreamer::emitAbsoluteSymbolDiff(const MCSymbol *Hi,
> >                                                const MCSymbol *Lo,
> >                                                unsigned Size) {
> > -  // If not assigned to the same (valid) fragment, fallback.
> > -  if (!Hi->getFragment() || Hi->getFragment() != Lo->getFragment() ||
> > -      Hi->isVariable() || Lo->isVariable()) {
> > +  Optional<uint64_t> Diff = absoluteSymbolDiff(Hi, Lo);
> > +  if (!Diff.hasValue()) {
> >      MCStreamer::emitAbsoluteSymbolDiff(Hi, Lo, Size);
> >      return;
> >    }
> >
> > -  EmitIntValue(Hi->getOffset() - Lo->getOffset(), Size);
> > +  EmitIntValue(*Diff, Size);
>
>
> I think you can write this as
>
> if (Optional<uint64_t> Diff = ...) {
>   EmitIntValue(*Diff, Size);
>   return;
> }
> MCStreamer::emitAbsoluteSymbolDiff(Hi, Lo, Size);
>

Done.

This patch does two things:
>
> * Remove the manual accounting and replace it with using assembler
>   expressions.
> * Use uleb.
>
> The first part should be an awesome nop cleanup. Could it be split out
> of this patch?
>

Yeah, that should be possible.

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.

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.

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.

Thanks,
-Ryan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180206/f75fae47/attachment.html>