[llvm-commits] [llvm] r54751 - in /llvm/trunk: Makefile.rules include/llvm/CodeGen/FastISel.h lib/CodeGen/SelectionDAG/FastISel.cpp utils/TableGen/FastISelEmitter.cpp utils/TableGen/FastISelEmitter.h utils/TableGen/TableGen.cpp

[Chris Lattner]

On Aug 19, 2008, at 2:06 PM, Dan Gohman wrote:
>> That also doesn't really help necessarily, because lli has to do
>> selection as well.  I guess it wins if you only execute a small
>> percentage of the code being compiled though.
>
> I specifically meant an interpreter, which wouldn't need a distinct
> selection phase.

Ah, gotcha.  Still seems kind of tangential to the file.

>>>
>>> +  typedef std::map<MVT::SimpleValueType, InstructionMemo> TypeMap;
>>> +  typedef std::map<std::string, TypeMap> OpcodeTypeMap;
>>> +  typedef std::map<OperandsSignature, OpcodeTypeMap>
>>> OperandsOpcodeTypeMap;
>>> +  OperandsOpcodeTypeMap SimplePatterns;
>>
>> Maybe it would be cleaner to make fastiselemitter be a class with a
>> bunch of methods?  That would give you a clean way to share these
>> typedefs and still allow you to split up "run" into multiple  
>> different
>> methods.  Splitting it up into methods gives the opportunity to pick
>> useful names for each unit.  This works well for dagiselemitter at
>> least.
>
> Yep. OperandsSignature is an example of code that used to all be
> done inline that I've refactored. I plan to keep doing this as I
> proceed.

Ok

>> It would be useful to know why your rejecting each of these.  I think
>> this will filter out the X86 shift patterns (which use CL), for
>> example.  Is this a temporary or permanent approach?
>
> It's temporary; I am assuming that a design that fundamentally
> excludes shift instructions would evoke consternation.

hehe :)

>>>
>>> +  SelectInstructions(BasicBlock::iterator Begin,
>>> BasicBlock::iterator End,
>>> +                     DenseMap<const Value*, unsigned> &ValueMap);
>>
>> If you change this to take two Instruction*'s as an inclusive range  
>> to
>> emit, you can avoid #including BasicBlock.h.  I agree this is  
>> somewhat
>> ugly though and probably not worth it.
>
> I'm leaving it as is for now.

Probably for the best, ok

>

Thanks Dan!

-Chris