[llvm-commits] Speeding up instruction selection

Roman Levenstein romix.llvm at googlemail.com
Wed Mar 26 05:45:16 PDT 2008


Hi Dan,

2008/3/25, Dan Gohman <gohman at apple.com>:
> Hi Roman,
>
>  I've run the MultiSource tests and most of the External/SPEC tests
>  with this patch and they all passed.

Dan, Thank you very much for testing it!!!

> I do have more comments below,
>  though they're mostly cosmetic and I think it's fine if you want to
>  commit the patch as is and address them separately.

I first commit the patch as is with only minor changes, i.e.
getUser/setUser change.

>  On Mar 25, 2008, at 3:36 AM, Roman Levenstein wrote:
>  >
>  > XFAIL: /opt/llvm/test/Analysis/ScalarEvolution/2008-02-11-
>  > ReversedCondition.ll
>  > FAIL: /opt/llvm/test/C++Frontend/2006-11-06-StackTrace.cpp
>  > FAIL: /opt/llvm/test/C++Frontend/2008-02-13-sret.cpp
>  > FAIL: /opt/llvm/test/CFrontend/2008-01-28-PragmaMark.c
>  > FAIL: /opt/llvm/test/CFrontend/2008-02-11-AnnotateBuiltin.c
>  > FAIL: /opt/llvm/test/CFrontend/2008-03-03-CtorAttrType.c
>  > FAIL: /opt/llvm/test/CFrontend/2008-03-05-syncPtr.c
>  > XFAIL: /opt/llvm/test/CodeGen/Alpha/mul5.ll
>  > XFAIL: /opt/llvm/test/CodeGen/Generic/2007-04-14-EHSelectorCrash.ll
>  > for PR1508,1326
>  > XFAIL: /opt/llvm/test/CodeGen/Generic/2007-11-21-UndeadIllegalNode.ll
>  > XFAIL: /opt/llvm/test/Transforms/InstCombine/zext-fold.ll for PR1570
>
>
> XFAIL means the test is expected to fail, so you can safely
>  ignore those. And it's fine to ignore these *Frontend failures
>  here.

OK. Thanks for making it clear for me.


>  >
>  >> Also, I have a few quick questions on the patch itself:
>  >>
>  >>> +  /// Make it possible to use SDOperandImpl as an SDNode
>  >>> +  operator SDNode * () { return parent; }
>  >> I'm confused by this. It seems confusing to have SDOperand be
>  >> implicitly convertible to SDNode. I'm pretty sure it wasn't before.
>  >
>  > Before my patch, the use_iterator was defined like this:
>  >  typedef SmallVector<SDNode*,3>::const_iterator use_iterator;
>  > And in many places in different code files it was typically used like:
>  >  SDNode *User = *UI;
>  >
>  > With my changes, *UI now returns actually an SDOperand. Therefore,
>  > either all of the places as the one above should be changed or there
>  > should be a casting operator defined to convert an SDOperand into an
>  > SDNode *. I wanted to change the code as little as possible and have
>  > chosen the second approach. But if you think it can be dangerous, I
>  > can go for the first one. Then every code line of the form like this:
>  >  SDNode *User = *UI;
>  > will be replaced by:
>  >  SDNode *User = UI->getParent();
>
>
> The implicit conversion does seem dangerous to me. And it's
>  different in a subtle way from the analogous constructs
>  in the high-level IR, where the Use class implicitly converts
>  to a Value* for the value being used, not the user of the
>  value.
>
>  I'd prefer for use_iterator's operator* and operator-> to
>  be similar to those in value_use_iterator. Renaming getParent
>  to getUser would aid the analogy. Or if that's not
>  possible, I think making the calls explicit would be better
>  than the implicit conversion.

>  >
>  >> Can you explain what this is doing?
>  >>
>  >>>     for (unsigned i = 0; i != NumOps; ++i) {
>  >>>       OperandList[i] = Ops[i];
>  >>> -      Ops[i].Val->Uses.push_back(this);
>  >>> +      OperandList[i].setParent(this);
>  >>> +      Ops[i].Val->addUse(OperandList[i]);
>  >>> +      ++Ops[i].Val->UsesSize;
>  >>>     }
>  >>
>  >> It looks like the new addUse call is making each operand
>  >> a use of itself here. That doesn't seem right.
>  >
>  > Well it adds each operand as a use to the Ops[i].Val, which is the
>  > SDNode this operand points to. And this is exactly what should be
>  > done.
>
>
> Ok, I get it now. The names of various things are confusing
>  me here.
>
>  Please change SDOperand's getParent/setParent to getUser/setUser,
>  as the term "parent" is somewhat ambiguous here.

Done.

>  SDOperand and SDOperandImpl are not very descriptive of
>  their new respective roles. How about SDUse instead of
>  SDOperand, and SDValue instead of SDOperandImpl?

Hmm. I agree with you, that better names could be more descriptive. I
even tried to do the renaming as you proposed. These are my findings:
 - about a few thausends of places needto  be changed in about 15
source files, thus producing a huge patch.
 - tablegen is likely to be changed, since it produces some *.inc
files for the instruction selector, where SDOperand is used.

IMHO, such a renaming change would affect too much code, so I'd like
to avoid doing it. But if you still tell me to go for more descriptive
names, I could do it.
How should we proceed here?

>  Also, the patch has some comments that mention
>  "SDUseOperand". Please update these.

Done.

>  The patch adds a specialization for DenseMapInfo for
>  SDOperandImpl, but it leaves the specialization for SDOperand
>  in place. This is somewhat worrisome, as SDOperand now has a
>  significant field, the user, that isn't considered in the hash
>  and comparison functions. Offhand, I would guess that all of
>  the DenseMaps that use SDOperand keys should be changed to use
>  SDOperandImpl keys. Then DenseMapInfo<SDOperand> can just be
>  removed, so we don't have to worry about it.

Done.

Thank you very much for your review and approval.

BTW, with this patch in place, may of the ReplaceAll... functions can
be probably implemented more efficiently (though they are much faster
with this patch already), e.g. by using bulk replacements or something
like this. Any ideas about what can be done?

-Roman

P.S.
BTW, this patch of mine for speeding-up the instruction selection is
not reviewed/tested yet. Could you have a look?
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080303/059215.html



More information about the llvm-commits mailing list