[llvm-commits] [llvm] r173342 - in /llvm/trunk: lib/Transforms/Utils/SimplifyCFG.cpp test/Transforms/SimplifyCFG/SpeculativeExec.ll

[Chandler Carruth]

On Fri, Jan 25, 2013 at 10:16 AM, Jakob Stoklund Olesen <stoklund at 2pi.dk>wrote:

>
> On Jan 24, 2013, at 4:39 AM, Chandler Carruth <chandlerc at gmail.com> wrote:
>
> > Author: chandlerc
> > Date: Thu Jan 24 06:39:29 2013
> > New Revision: 173342
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=173342&view=rev
> > Log:
> > Plug TTI into the speculation logic, giving it a real cost interface
> > that can be specialized by targets.
> >
> > The goal here is not to be more aggressive, but to just be more accurate
> > with very obvious cases. There are instructions which are known to be
> > truly free and which were not being modeled as such in this code -- see
> > the regression test which is distilled from an inner loop of zlib.
>
> Hi Chandler,
>
> It is important to realize how profoundly clueless the IR passes are about
> execution speed. The obvious cases you mention here could easily cause
> regressions. (Just like the existing heuristics can).
>

I think I'm relatively aware here, and in fact really do agree.

But this patch isn't substantively changing what's going on here. We
*already* had a completely broken and terrible cost model, and did
brain-dead if-conversion on it. This patch makes the if-conversions
performed at the IR level somewhat more *consistent*. That was the goal.

The concrete code that motivated me to get here was code that
would oscillate between being if-converted here and not being if-converted
here because of "no-op" transforms elsewhere in the IR optimizer:

--------
  %cmp = icmp ... i16 %a, %b
  br i1 %cmp, label %true, label %false

true:
  %sub = add i16 %a, %b
  br label %false

false:
  %phi = i16 [ %a, ... ], [ %sub, %true ]
--------
became:
--------
  %a.conv = zext i16 %a to i32
  %b.conv = zext i16 %b to i32
  %cmp = icmp ... i32 %a.conv, %b.conv
  br i1 %cmp, label %true, label %false

true:
  %sub = add i32 %a.conv, %b.conv
  %sub.conv = trunc i32 %sub to i16
  br label %false

false:
  %phi = i16 [ %a, ... ], [ %sub.conv, %true ]
--------

The first one got if-converted by SimplifyCFG, the second one didn't. The
promotion to i32 happened because of code elsewhere, it happened sometimes
and not others, sometimes because of inlining. And the code is then handled
by the rest of the optimizer completely differently.

The presence of a 'trunc' in the conditional basic block shouldn't be the
thing that enables or disables this. All I wanted out of this patch was
improved stability and consistency on x86.


Now, why not just nuke the if-conversion altogether? Because, much to my
regret, early if-conversion isn't enabled on x86, so we don't end up fixing
this later. To make matters worse, other *IR* level passes were written
assuming these selects would be formed... something I find very unfortunate.

Dan and I have been discussing various different approaches to solving the
fundamental problem here and avoiding any such hacks, but I think that the
badness you're arguing against is badness that was *already here*, and
which we don't yet have a good alternative for. We should absolutely find a
better way, but I don't think that should preclude some bandaids going in
in the interim.


I also added a big comment to the top of the function to try to warn people
away from spending too much time growing more functionality here until we
have a more principled solution in place. (I checked the LNT bots and saw
no regressions from my work fwiw...)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130125/1878658b/attachment.html>