[llvm-commits] SSI patch
Andre Tavares
andrelct at dcc.ufmg.br
Mon Jun 29 13:33:27 PDT 2009
Nick Lewycky wrote:
> Andre Tavares wrote:
>
>> Hello,
>>
>> I'm submiting a patch that will be responsible to create SSI
>> representation on demand. This representation conforms with the current
>> SSA, and will not break other optimizations. It only inserts some phi
>> functions on the code. A brief description is below, and the code is
>> attached.
>>
>> // This pass converts a list of variables to the Static Single Information
>> // form. This is a program representation described by Scott Ananian in his
>> // Master Thesis: "The Static Single Information Form (1999)".
>> // We are building an on-demand representation, that is, we do not convert
>> // every single variable in the target function to SSI form. Rather, we
>> receive
>> // a list of target variables that must be converted. We also do not
>> // completely convert a target variable to the SSI format. Instead, we only
>> // change the variable in the points where new information can be attached
>> // to its live range, that is, at branch points.
>>
>
> // Test if there is a need to transform to SSI
> if (needConstruction.count() > 0) {
>
> Use !needConstruction.empty() .
>
Yep, changed to any, as you mentioned below somewhere.
> void SSI::insertSigmaFunctions(std::vector<Instruction *> & value) {
> for (unsigned i = 0; i < num_values; ++i) {
> if (needConstruction[i]) {
>
> Just put "if (!needConstruction[i]) continue;" to avoid indenting
> everything another level.
>
ok
> for (; begin != end; ++begin) {
> // Test if the Use of the Value is in a comparator
> CmpInst * CI;
> if ((CI = dyn_cast<CmpInst> (begin)) && isUsedInTerminator(CI)) {
>
> How about:
> for (; begin != end; ++begin) {
> CmpInst *CI = dyn_cast<CmpInst>(begin);
> if (!CI || !isUsedInTerminator(CI)) {
> instead?
>
ok. Isn't it without !. Like
if (CI || !isUsedInTerminator(CI)) {
> In insertPhiFunctions:
>
> std::set<BasicBlock *> * BB_visited = new std::set<BasicBlock *>();
> while (...) { ...
> free(BB_visited);
> }
>
> How about allocating std::set<BasicBlock *> BB_visited on the stack and
> calling BB_visited.clear() each iteration through the loop?
>
>
Done already, just didn't send the latest version. Thanks.
> // Test if has not yet visited this node and if the
> // original definition dominates this node
> if (BB_visited->insert(BB_dominated).second && value_original[i]
> != BB_dominated
> && DT_->dominates(value_original[i], BB_dominated)
> && dominateAny(BB_dominated, value[i])) {
>
> I think the "value_original[i] != BB_dominated &&
> DT_->dominates(value_original[i], BB_dominated)" part can be rewritten
> as "DT_->properlyDominates(value_original[i], BB_dominated)".
>
>
Of coarse. Missed that.
> Also, please fit the operators on the end of the line.
>
> In SSI::rename:
> // For SSI_SIG (b = PHI(a)), b is a new definition to a. So we need to
> "new definition *of a"?
> // Then store bin the value_stack as the new definition of a.
> Not sure what the means.
>
I mean that b is a sigma of a, so it is a new definition of a. But I
changed that part to look better.
> TerminatorInst * TI = BB->getTerminator();
> for (unsigned i = 0; i < TI->getNumSuccessors(); ++i) {
> BasicBlock * BB_succ = TI->getSuccessor(i);
>
> Perhaps you wanted:
> for (succ_iterator SI = succ_begin(BI), SE = succ_end(B); SI != SE;
> ++SI) {
> ? You need to #include "llvm/Support/CFG.h" for that.
>
>
ok.
> // This loop calls rename on all children from this block. This time
> children
> // refers to a successor block in the dominance tree.
>
> In other words, it calls rename on all immediately dominated blocks?
>
>
yes.
> if (defined->count() > 0) {
>
> Use "if (defined->any()) {". Count isn't particularly expensive but
> any() is cheaper.
>
>
>
ok
> void SSI::substituteUse(Instruction * I) {
> for (unsigned i = 0; i < I->getNumOperands(); ++i) {
> Value * operand = I->getOperand(i);
> for (unsigned j = 0; j < num_values; ++j) {
> if (operand == value_stack[j].front() && I !=
> value_stack[j].back()) {
>
> It's not clear to me what the if-statement is actually doing, and
> whether it's any different from just using "if (i != j) continue;".
>
There are two for here. The outside for iterates in all operands of I,
and the inside in all variables I received. The first part of the if is
testing if the operand of the moment is the variable of the moment. I
can't use "if (i != j) continue;", because i and j are used to iterate
in different things. Was clear?
> if ((I_phi = dyn_cast<PHINode> (I)) && (vs_phi = dyn_cast<PHINode> (
> value_stack[j].back())) && value_stack[j].back()->getParent()
> == I->getParent()) {
>
> This needs to be line-flowed better. Here's my suggestion:
> if ((I_phi = dyn_cast<PHINode>(I)) &&
> (vs_phi = dyn_cast<PHINode> (value_stack[j].back())) &&
> value_stack[j].back()->getParent() == I->getParent()) {
> which keeps each 'phrase' on one line.
>
Changed to
PHINode * I_phi = dyn_cast<PHINode> (I);
PHINode * vs_phi = dyn_cast<PHINode> (value_stack[j].back());
if (I_phi && vs_phi && value_stack[j].back()->getParent()
== I->getParent()) {
> /// Test if the BasicBlock BB dominates any use or definition of value.
> ///
> bool SSI::dominateAny(BasicBlock * BB, Instruction * value) {
>
> This whole method should work by a) checking whether the Instruction
> itself is dominator (if so, so are all non-phi uses, so return true
> fast) else just loop over the users who are PHINodes and test them only.
>
>
Don't get what you mean here. I have to test all users of the
instruction not only the PHINodes, correct?
This method is testing if the value is alive in BB. As there is no
LiveVariableAnalysis here I used Dominance.
> Break SSI::getPosition up into getPositionPhi and getPositionSigma. All
> callers put a constant value into compare anyways, and there's no way
> that copying an std::map like that can be cheap.
>
>
ok
> In SSI::isUsedInTerminator, we know that the comparison field of a
> BranchInst or SwitchInst is going to be field 0. Just make sure
> getNumOperands() != 0 then look at field 0. No loop needed.
>
>
great.
> Overall, looks good! I like your choice of Ananian's algorithm over
> Singer's because it avoids requiring reverse dominfo (yay!). I didn't
> see anything wrong with the algorithm details.
>
> The one thing to remember is that when placing sigma's in the first
> place you use a very simple flow of "is it a CmpInst which flows to a
> TerminatorInst". That's probably good enough for ABCD but not for a
> GCC-like VRP algorithm. You don't need to worry about that though,
> whoever tried to implement a GCC-like VRP gets to worry about it.
>
> Nick
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
--
Andre Tavares
Master Student in Computer Science - UFMG - Brasil
http://dcc.ufmg.br/~andrelct
More information about the llvm-commits
mailing list