[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 clattner at apple.com
Sat Aug 16 17:07:50 PDT 2008


On Aug 13, 2008, at 1:19 PM, Dan Gohman wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=54751&view=rev
> Log:
> Initial checkin of the new "fast" instruction selection support. See
> the comments in FastISelEmitter.cpp for details on what this is.
> This is currently experimental and unusable.

Nice!

> +++ llvm/trunk/utils/TableGen/FastISelEmitter.h Wed Aug 13 15:19:35  
> 2008
> @@ -0,0 +1,38 @@
> +#ifndef FASTISEL_EMITTER_H
> +#define FASTISEL_EMITTER_H
> +
> +#include "TableGenBackend.h"
> +#include "CodeGenDAGPatterns.h"
> +#include <set>

No need for <set>?

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> +++ llvm/trunk/utils/TableGen/FastISelEmitter.cpp Wed Aug 13  
> 15:19:35 2008
> +// If compile time is so important, you might wonder why we don't  
> just
> +// skip codegen all-together, emit LLVM bytecode files, and execute  
> them
> +// with an interpreter. The answer is that it would complicate  
> linking and
> +// debugging, and also because that isn't how a compiler is  
> expected to
> +// work in some circles.

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.

> +// If you need better generated code or more lowering than what this
> +// instruction selector provides, use the SelectionDAG (DAGISel)  
> instruction
> +// selector instead. If you're looking here because SelectionDAG  
> isn't fast
> +// enough, consider looking into improving the SelectionDAG  
> infastructure
> +// instead. At the time of this writing there remain several major
> +// opportunities for improvement.

I think a better and more permanent way of putting this is "only  
consider enhancing the fast isel if you want to make it produce code  
faster.  Changes to make the fast isel produce better code more slowly  
will be considered to be a step backward.

>
> +
> +struct OperandsSignature {

Please give a brief comment about what each of these classes is for.

> +void FastISelEmitter::run(std::ostream &OS) {
...

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

> +  for (CodeGenDAGPatterns::ptm_iterator I = CGP.ptm_begin(),
> +       E = CGP.ptm_end(); I != E; ++I) {
> +    const PatternToMatch &Pattern = *I;
> +
> +    // For now, just look at Instructions, so that we don't have to  
> worry
> +    // about emitting multiple instructions for a pattern.
> +    TreePatternNode *Dst = Pattern.getDstPattern();
> +    if (Dst->isLeaf()) continue;
> +    Record *Op = Dst->getOperator();
> +    if (!Op->isSubClassOf("Instruction"))
> +      continue;
> +    CodeGenInstruction &II = CGP.getTargetInfo().getInstruction(Op- 
> >getName());
> +    if (II.OperandList.empty())
> +      continue;
> +    Record *Op0Rec = II.OperandList[0].Rec;
> +    if (!Op0Rec->isSubClassOf("RegisterClass"))
> +      continue;
> +    const CodeGenRegisterClass *DstRC =  
> &Target.getRegisterClass(Op0Rec);
> +    if (!DstRC)
> +      continue;

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?

> +    // For now, filter out instructions which just set a register to
> +    // an Operand, like MOV32ri.
> +    if (InstPatOp->isSubClassOf("Operand"))
> +      continue;

This is a great description which makes it easier to follow the code.

> +
> +    // Check all the operands. For now only accept register operands.
> +    OperandsSignature Operands;
> +    for (unsigned i = 0, e = InstPatNode->getNumChildren(); i != e;  
> ++i) {
> +      TreePatternNode *Op = InstPatNode->getChild(i);
> +      if (!Op->isLeaf())
> +        goto continue_label;
> +      DefInit *OpDI = dynamic_cast<DefInit*>(Op->getLeafValue());
> +      if (!OpDI)
> +        goto continue_label;
> +      Record *OpLeafRec = OpDI->getDef();
> +      if (!OpLeafRec->isSubClassOf("RegisterClass"))
> +        goto continue_label;
> +      const CodeGenRegisterClass *RC =  
> &Target.getRegisterClass(OpLeafRec);
> +      if (!RC)
> +        goto continue_label;
> +      if (Op->getTypeNum(0) != VT)
> +        goto continue_label;
> +      Operands.Operands.push_back("r");
> +    }

Please split this loop out into a helper function, this will let you  
get rid of the gotos, by using something like:

       if (GetFooInfo(InstPatNode, Operands))
         continue;


> +++ llvm/trunk/include/llvm/CodeGen/FastISel.h Wed Aug 13 15:19:35  
> 2008
> @@ -0,0 +1,71 @@


> +
> +/// This file defines the FastISel class. This is a fast-path  
> instruction

Plz update/remove the first sentence.

> +/// selection class that generates poor code and doesn't support  
> illegal
> +/// types or non-trivial lowering, but runs quickly.
> +class FastISel {
> +  MachineBasicBlock *MBB;
> +  MachineFunction *MF;
> +  const TargetInstrInfo *TII;
> +
> +public:
> +  FastISel(MachineBasicBlock *mbb, MachineFunction *mf,
> +           const TargetInstrInfo *tii)
> +    : MBB(mbb), MF(mf), TII(tii) {}
> +
> +  /// SelectInstructions - Do "fast" instruction selection over the
> +  /// LLVM IR instructions in the range [Begin, N) where N is either
> +  /// End or the first unsupported instruction. Return N.
> +  /// ValueMap is filled in with a mapping of LLVM IR Values to
> +  /// register numbers.
> +  BasicBlock::iterator
> +  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.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp (added)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp Wed Aug 13  
> 15:19:35 2008
> @@ -0,0 +1,104 @@

> +BasicBlock::iterator
> +FastISel::SelectInstructions(BasicBlock::iterator Begin,  
> BasicBlock::iterator End,
> +                             DenseMap<const Value*, unsigned>  
> &ValueMap) {

I like this interface.  Is the idea that you only use selectiondag  
selection on the specific *instructions* that fastisel can't handle,  
instead of whole blocks?

> +  BasicBlock::iterator I = Begin;
> +
> +  for (; I != End; ++I) {
> +    switch (I->getOpcode()) {
> +    case Instruction::Add: {
> +      unsigned Op0 = ValueMap[I->getOperand(0)];
> +      unsigned Op1 = ValueMap[I->getOperand(1)];
> +      MVT VT = MVT::getMVT(I->getType(), /*HandleUnknown=*/true);
> +      if (VT == MVT::Other || !VT.isSimple()) {
> +        // Unhandled type. Halt "fast" selection and bail.
> +        return I;
> +      }
>
> +      unsigned ResultReg = FastEmit_rr(VT.getSimpleVT(), ISD::ADD,  
> Op0, Op1);

I think this needs to 'return I' if ResultReg is 0.

>
> +unsigned FastISel::FastEmitInst_(unsigned MachineInstOpcode,
> +                                    const TargetRegisterClass* RC) {
> +  MachineRegisterInfo &MRI = MF->getRegInfo();
> +  const TargetInstrDesc &II = TII->get(MachineInstOpcode);
> +  MachineInstr *MI = BuildMI(*MF, II);
> +  unsigned ResultReg = MRI.createVirtualRegister(RC);
> +
> +  MI->addOperand(MachineOperand::CreateReg(ResultReg, true));
> +  MBB->push_back(MI);

Why not just "BuildMI(MBB, II, ResultReg);"

> +  return ResultReg;
> +}
> +
> +unsigned FastISel::FastEmitInst_r(unsigned MachineInstOpcode,
> +                                  const TargetRegisterClass *RC,
> +                                  unsigned Op0) {
> +  MachineRegisterInfo &MRI = MF->getRegInfo();
> +  const TargetInstrDesc &II = TII->get(MachineInstOpcode);
> +  MachineInstr *MI = BuildMI(*MF, II);
> +  unsigned ResultReg = MRI.createVirtualRegister(RC);
> +
> +  MI->addOperand(MachineOperand::CreateReg(ResultReg, true));
> +  MI->addOperand(MachineOperand::CreateReg(Op0, false));
> +
> +  MBB->push_back(MI);

Why not just "BuildMI(MBB, II, ResultReg).addReg(Op0);"

Overall, very nice start.  Do you have hooks for SDISel to use this?   
If so, plz commit them as well under a flag.

-Chris



More information about the llvm-commits mailing list