[llvm-commits] Speeding up instruction selection

[Dan Gohman]

Hi Roman,

I've run the MultiSource tests and most of the External/SPEC tests
with this patch and they all passed. 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.

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.

>
>> 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.

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

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


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.

Thanks,

Dan