[llvm-commits] Speeding up instruction selection

[Dan Gohman]

On Mar 26, 2008, at 5:45 AM, Roman Levenstein wrote:
> 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!!!

You're welcome. Thank you for developing it! With all the recent
talk about whole-function or superblock DAGs, scalability is
expected to be especially important.

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

Ok.

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

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

Looking at this more, I think there's a problem we should fix
here. All those thousands of mentions of SDOperand are places
that shouldn't need to know who the user is, which means that
they ought to be using SDOperandImpl instead of SDOperand,
given the way those classes are currently defined.

I would suggest starting by renaming the current SDOperand to
SDUse, and rename SDOperandImpl to SDOperand, leaving most
of the thousands of SDOperand mentions untouched. The
DenseMaps that previously changed from SDOperand to SDOperandImpl
can go back to SDOperand then. This will likely require some care
to get right, but it shouldn't require huge quantities of
changes.

Once that's done and we're comfortable it, we can look at taking
the second step, which would be a mass-rename of SDOperand
to SDValue.

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

For the moment, I think it's best to wait until the naming
issues are straightened out.

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

Yes, thanks. I hope to get to it soon. Thanks for your patience.

Dan