[LLVMdev] [patch] Pluggable Coalescers

[Evan Cheng]

Hi David,

Thanks for this patch!

Some comments:

1. typedef std::set<const LiveInterval *> IntervalSet;
     Please use SmallPtrSet instead.

2.
+    virtual void mergeIntervals(const LiveInterval &a,
+                                const LiveInterval &b,
+                                const MachineInstr &copy) {};

I find the name misleading. It's not actually performing the merging,  
right? It's updating the interference graph to reflect the merge.  
Please choose a more descriptive name.

3.

+    /// Allow the register allocator to communicate when it doesn't
+    /// want a copy coalesced.  This may be due to assumptions made by
+    /// the allocator about various invariants and so this question is
+    /// a matter of legality, not performance.  Performance decisions
+    /// about which copies to coalesce should be made by the
+    /// coalescer.
+    virtual bool okToCoalesce(const MachineInstr &inst) const {
+      return(true);
+    }

I think we discussed this early but please remind me. Why is this  
necessary? Why isn't interfere() sufficient test? Also, I would prefer  
a name like isLegalToCoalesce over okToCoalesce.

4. Is it necessary to separate class InterferenceData out from  
RegisterCoalescer.h?

5.
+    /// Run the coalescer on this function, providing interference
+    /// data to query.  Return whether we removed any copies.
+    virtual bool coalesceFunction(MachineFunction &mf,  
InterferenceData &ifd) = 0;

80 col violation.

6.
+    /// doWork - The main coalescing algorithm.  Return whether any
+    /// copies were coalesced.
+    bool doWork(MachineFunction &mf);

Please rename it to something like performCoalescing. Also, is this  
defined anywhere?

7.  About class LinearScanInterferenceData. Since it's just an  
example, perhaps it should go into comments or a README file instead.

Evan

On Aug 20, 2007, at 1:58 PM, David A. Greene wrote:

> <pluggable_coalescer.patch>