[llvm-commits] More aggressive speculation in SimplifyCFG

[Hal Finkel]

Krzysztof,

A few quick remarks:

 static cl::opt<bool>
+NewGenSel("new-gen-sel", cl::Hidden, cl::init(false),
+    cl::desc("New code to generate select instructions more aggressively"));
+
+static cl::opt<unsigned>
+BBSpecThreshold("bb-spec-threshold", cl::Hidden, cl::init(3),
+    cl::desc("Threshold for the cost of speculating a basic block"));

I think that these parameters should be moved into TTI so that they can be customized by the targets. Also, I don't like "new-gen-sel" because "new" is not descriptive. Why don't we just call it aggressive speculation (and move it into the default TTI implementation).

+static unsigned SelCount = 0;
...
+    else
+      SelName = Twine("sel") + Twine(++SelCount);

I don't think that this is needed. IIRC, duplicate names automatically get numbers appended.

+  // Be conservative for now. FP select instruction can often be expensive.
+  Value *BrCond = BI->getCondition();
+  if (isa<FCmpInst>(BrCond))
+    return false;

This should also be a target option (not a built-in assumption).

+  typedef std::pair<Value*, Value*> ValuePair;
+  typedef std::map<PHINode*, ValuePair> PhiMap;

Can't you use a DenseMap here?

+  return NewGenSel ? SpeculativelyExecuteBB_new(BI, BB1)
+                   : SpeculativelyExecuteBB_old(BI, BB1);

These function names don't follow the naming convention. Also, new and old are not descriptive.

-; RUN: opt < %s -simplifycfg -S | FileCheck %s
+; RUN: opt < %s -simplifycfg -new-gen-sel=false -S | FileCheck %s

Several tests have this change. Why? (false is the default).

+sideu:
+  %u1 = add i32 %y, -1
+  %u2 = sub i32 %x, %u1
+  br label %join
+join:
+; CHECK: select

I'd prefer if the tests checked for more than just the presence of the select. Is there a reason not to include the entire expected output?

Thanks again,
Hal

----- Original Message -----
> From: "Krzysztof Parzyszek" <kparzysz at codeaurora.org>
> To: "llvm-commits at cs.uiuc.edu commits" <llvm-commits at cs.uiuc.edu>
> Cc: sabre at nondot.org
> Sent: Tuesday, January 22, 2013 5:42:34 PM
> Subject: [llvm-commits] More aggressive speculation in SimplifyCFG
> 
> This is a patch that enables generating "select" instructions more
> aggressively in SimplifyCFG.  It was written a while ago to convert
> more
> instances of conditional code into a straight-line code.  As
> presented
> in the attached patch, it is disabled by default.
> 
> Please take a look and share comments if you have any.
> 
> -Krzysztof
> 
> 
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>