[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

[Dan Gohman]

On Aug 16, 2008, at 5:07 PM, Chris Lattner wrote:

> 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>?

Nope.

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

I specifically meant an interpreter, which wouldn't need a distinct
selection phase.

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

Done.

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

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.

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

It's temporary; I am assuming that a design that fundamentally
excludes shift instructions would evoke consternation.

>
>
>> +    // 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;

Ok.

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

Done.

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

I'm leaving it as is for now.

>
>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> = 
>> =====================================================================
>> --- 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?

Basically yes. The idea is that "fast" mode would start out
processing a block with fastisel and then switch over to
selectiondag if it hits something it can't handle.

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

Good catch. Fixed.

>
>
>>
>> +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);"

Ok.

>
>> +  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);"

Ok.

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

Ok, I'll get there soon.

Dan