[PATCH] D60982: [SimplifyCFG] Use lookup tables when they are more space efficient or a huge speed win.

[James Molloy via llvm-commits]

Ah, thanks for the clarification. If that's the case I understand the
intent now!

On Thu, 25 Apr 2019, 20:19 Nikita Popov via Phabricator, <
reviews at reviews.llvm.org> wrote:

> nikic added inline comments.
>
>
> ================
> Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5386
> +    Value *V;
> +    if (match(SI->getCondition(), m_Add(m_Value(V), m_ConstantInt(CAdd)))
> ||
> +        match(SI->getCondition(), m_Sub(m_Value(V), m_ConstantInt(CSub)))
> ||
> ----------------
> jmolloy wrote:
> > shawnl wrote:
> > > jmolloy wrote:
> > > > This assumes whatever constant add/sub/xor was planted still exists.
> There's no guarantee of that; for example if V is a constant, IRBuilder
> would have constant folded immediately.
> > > >
> > > > In general unstitching code like this is inherently problematic.
> Note that isn't the same as planting an inverting code sequence - that's
> fine. We can use the optimizer to clean the duplicated stuff up and
> everyone is happy. That is much preferable to hamhandedly unstitching stuff
> and looking for patterns of code you've just emitted. It also adds an
> implicit contract between different parts of SimplifyCFG that I guarantee
> someone will miss when they update this code :)
> > > The Xor wasn't added by this stuff. The problem is that this pass gets
> run multiple times, sometimes without the table generation (because it can
> make the code analyzable). There is nowhere else this optimization can go,
> because it is an optimization specific to switch statements, where the
> operands can be re-ordered arbitrarily. Also, there is no assumption that
> these things are there. It simply sees if they are there and if so removed
> them and then returns true so that the other optimizations run before we
> continue.
> > >
> > > So I feel this is a planted inverting code sequence: this is not
> "unstitching" (and the code does not generate xors but does remove them):
> it is an optimization specific to covered tables.
> > My worry about this code is that it doesn't assert the preconditions or
> postconditions. AFAICS, it detects any Add, Sub or Xor and does some
> transform on them. Why is this correct in all cases? What if one of these
> instructions were missed? What if there is a sequence of these
> instructions, is it correct to just adjust a subset? What if they were
> reordered or constant folded?
> >
> > The code doesn't explain exactly what it's checking for, who generates
> it and why it's correct to remove (and what happens if you have a false
> negative or false positive).
> >
> > Note I'm not saying it's wrong, or perhaps it's the only way to do this,
> but with the lack of comment it remains a code smell at the moment and it's
> difficult for me to suggest an alternative.
> I think the idea here is that given two bijective (total) functions f(x)
> and lookup(y), then lookup(f(x)) is also a bijective function that can be
> implemented as a new lookup table lookup_f(x). In this case and, sub and
> xor with a constant are the f(x)s.
>
> It seems like this could be an independent and more general optimization
> though, that works on any table lookup.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D60982/new/
>
> https://reviews.llvm.org/D60982
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190425/adf0e57f/attachment.html>