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

[Chris Lattner]

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

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;


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.  Please put DomainValue in an anonymous namespace.

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

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

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100331/113129a4/attachment.html>