[llvm-commits] [llvm] r99848 - in /llvm/trunk/lib/Target/X86: SSEDomainFix.cpp X86InstrInfo.cpp X86InstrInfo.h

[Jakob Stoklund Olesen]

On Mar 31, 2010, at 1:35 PM, Chris Lattner wrote:

> On Mar 29, 2010, at 4:24 PM, Jakob Stoklund Olesen wrote:
>> Author: stoklund
>> Date: Mon Mar 29 18:24:21 2010
>> New Revision: 99848
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=99848&view=rev
>> Log:
>> Basic implementation of SSEDomainFix pass.
> 
> Nice!  Looking through the implementation in mainline, some random thoughts:
> 
>   // Available domains. For an open DomainValue, it is the still possible
>   // domains for collapsing. For a collapsed DomainValue it is the domains where
>   // the register is available for free.
>   unsigned Mask;
> 
> Can this be named "AvailableDomains" or something?  Naming it mask is weird, particularly in:
> 
>  bool compat(unsigned mask) const {
>    return Mask & mask;
>  }

Yes, fair enough.

> Also, is there a better/more abstract representation that you can give this mask?  There is all sorts of bit twiddling and other stuff happening to these bits which just look like magic operations scattered around the code.  Defining a class like:
> 
> class AvailableDomains {
>   unsigned DomainMask;
> public:
> ...
>   unsigned getFirstDomain() const;

Wrapping a bit mask in a class seems like overkill, but it is a good idea to gather all the bit twiddling in DomainValue methods.

> Please name the predicates in DomainValue something like "isCompatible", "isCollapsed", "getFirstDomain", etc, to make it more obvious at use sites that this is a predicate.

Sure.

>  Please put DomainValue in an anonymous namespace.

Everything already is. Should I open a new namespace per class to make it more obvious?

> General question: why use calloc/free instead of new[]/delete[]?

I started with "new DomainValue*[NumRegs]", but it didn't zero-init my pointers (?). I'll gladly use new[]+std::fill instead.

> After this pass of changes is ready I'll take a closer look, thanks for working on this pass!

Thanks, Chris!

/jakob