[Draft Proposal] Refactor SILParser::parseSILInstruction

Evolution Discussion
1

Hi everyone,

I’m interested in taking SR-340 <Issues · apple/swift-issues · GitHub; issue:
"parseSILInstruction is horrible and makes me cry every time I see it. It is a method that is ~1900 lines with a huge switch in it. We should refactor it into a visitor structure. In fact it is large enough that we should consider moving it into its own file if it is possible."

Here is the first draft of the myl idea, how it could be approached:

1. We will define an abstract method on ValueBase class:

    class ValueBase
    {
    public:
        virtual void parse(class SILParseInstruction*) = 0;
    };
    
2. Each of the SIL instruction classes will override this method and will provide the following implementation:
    
    class SILArgument: public ValueBase
    {
    public:
        /*virtual*/void parse(SILParseInstruction*);
    };
    void SILArgument::parse(SILParseInstruction *i) {
        i->parseInstruction(this);
    }
    
    class PartialApplyInst: public ValueBase
    {
    public:
        /*virtual*/void parse(SILParseInstruction*);
    };
    void PartialApplyInst::parse(SILParseInstruction *i)
    {
        i->parseInstruction(this);
    }
    
3. We will define an abstract class for the callbacks from each of the SIL instruction classes

    class SILParseInstruction
    {
    public:
        virtual void parseInstruction(SILArgument*) = 0;
        virtual void parseInstruction(PartialApplyInst*) = 0;
    };

4. SILParser will implement these callbacks with the actual handling operations for each instruction.
    
    class SILParser: public SILParseInstruction
    {
    public:
        /*virtual*/void parseInstruction(SILArgument *r)
        {
            // Parse StringLiteralInst instruction
        }
        /*virtual*/void parseInstruction(PartialApplyInst *b)
        {
            // Parse PartialApplyInst instruction
        }
    };
    
5. The huge switch statement will be replaced with a single call:
  Opcode->parse(this);

What are your thoughts?

Sergey

2

Sounds great, but SIL is an implementation detail of the compiler and not formally part of the language spec. swift-dev would be a better venue for this discussion.

-Joe

···

On Jan 13, 2016, at 1:07 PM, Sergey Bolshedvorsky via swift-evolution <swift-evolution@swift.org> wrote:

Hi everyone,

I’m interested in taking SR-340 <Issues · apple/swift-issues · GitHub; issue:
"parseSILInstruction is horrible and makes me cry every time I see it. It is a method that is ~1900 lines with a huge switch in it. We should refactor it into a visitor structure. In fact it is large enough that we should consider moving it into its own file if it is possible."

Here is the first draft of the myl idea, how it could be approached:

1. We will define an abstract method on ValueBase class:

    class ValueBase
    {
    public:
        virtual void parse(class SILParseInstruction*) = 0;
    };
    
2. Each of the SIL instruction classes will override this method and will provide the following implementation:
    
    class SILArgument: public ValueBase
    {
    public:
        /*virtual*/void parse(SILParseInstruction*);
    };
    void SILArgument::parse(SILParseInstruction *i) {
        i->parseInstruction(this);
    }
    
    class PartialApplyInst: public ValueBase
    {
    public:
        /*virtual*/void parse(SILParseInstruction*);
    };
    void PartialApplyInst::parse(SILParseInstruction *i)
    {
        i->parseInstruction(this);
    }
    
3. We will define an abstract class for the callbacks from each of the SIL instruction classes

    class SILParseInstruction
    {
    public:
        virtual void parseInstruction(SILArgument*) = 0;
        virtual void parseInstruction(PartialApplyInst*) = 0;
    };

4. SILParser will implement these callbacks with the actual handling operations for each instruction.
    
    class SILParser: public SILParseInstruction
    {
    public:
        /*virtual*/void parseInstruction(SILArgument *r)
        {
            // Parse StringLiteralInst instruction
        }
        /*virtual*/void parseInstruction(PartialApplyInst *b)
        {
            // Parse PartialApplyInst instruction
        }
    };
    
5. The huge switch statement will be replaced with a single call:
  Opcode->parse(this);

What are your thoughts?

Sergey

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

3

Hi everyone,

I’m interested in taking SR-340 <Issues · apple/swift-issues · GitHub; issue:
"parseSILInstruction is horrible and makes me cry every time I see it. It is a method that is ~1900 lines with a huge switch in it. We should refactor it into a visitor structure. In fact it is large enough that we should consider moving it into its own file if it is possible."

Here is the first draft of the myl idea, how it could be approached:

1. We will define an abstract method on ValueBase class:

    class ValueBase
    {
    public:
        virtual void parse(class SILParseInstruction*) = 0;
    };
    
2. Each of the SIL instruction classes will override this method and will provide the following implementation:
    
    class SILArgument: public ValueBase
    {
    public:
        /*virtual*/void parse(SILParseInstruction*);
    };
    void SILArgument::parse(SILParseInstruction *i) {
        i->parseInstruction(this);
    }
    
    class PartialApplyInst: public ValueBase
    {
    public:
        /*virtual*/void parse(SILParseInstruction*);
    };
    void PartialApplyInst::parse(SILParseInstruction *i)
    {
        i->parseInstruction(this);
    }
    
3. We will define an abstract class for the callbacks from each of the SIL instruction classes

    class SILParseInstruction
    {
    public:
        virtual void parseInstruction(SILArgument*) = 0;
        virtual void parseInstruction(PartialApplyInst*) = 0;
    };

4. SILParser will implement these callbacks with the actual handling operations for each instruction.
    
    class SILParser: public SILParseInstruction
    {
    public:
        /*virtual*/void parseInstruction(SILArgument *r)
        {
            // Parse StringLiteralInst instruction
        }
        /*virtual*/void parseInstruction(PartialApplyInst *b)
        {
            // Parse PartialApplyInst instruction
        }
    };
    
5. The huge switch statement will be replaced with a single call:
  Opcode->parse(this);

What are your thoughts?

Sergey

···

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

4

Hey, Sergey. It definitely makes sense to refactor this, but I don't think putting methods on SILInstruction is the way to go about it. There's no reason most clients of SIL need to know anything about parsing; from a separation-of-concerns perspective it belongs in a separate header and very likely a separate component (i.e. not lib/SIL/).

If this were Swift, we could use an extension, but alas. :-)

That said, I'm not a heavy user of SIL, so someone else from the Swift team with more of a stake should comment.

Jordan

···

On Jan 13, 2016, at 13:27, Sergey Bolshedvorsky via swift-dev <swift-dev@swift.org> wrote:

Hi everyone,

I’m interested in taking SR-340 <Issues · apple/swift-issues · GitHub; issue:
"parseSILInstruction is horrible and makes me cry every time I see it. It is a method that is ~1900 lines with a huge switch in it. We should refactor it into a visitor structure. In fact it is large enough that we should consider moving it into its own file if it is possible."

Here is the first draft of the myl idea, how it could be approached:

1. We will define an abstract method on ValueBase class:

    class ValueBase
    {
    public:
        virtual void parse(class SILParseInstruction*) = 0;
    };
    
2. Each of the SIL instruction classes will override this method and will provide the following implementation:
    
    class SILArgument: public ValueBase
    {
    public:
        /*virtual*/void parse(SILParseInstruction*);
    };
    void SILArgument::parse(SILParseInstruction *i) {
        i->parseInstruction(this);
    }
    
    class PartialApplyInst: public ValueBase
    {
    public:
        /*virtual*/void parse(SILParseInstruction*);
    };
    void PartialApplyInst::parse(SILParseInstruction *i)
    {
        i->parseInstruction(this);
    }
    
3. We will define an abstract class for the callbacks from each of the SIL instruction classes

    class SILParseInstruction
    {
    public:
        virtual void parseInstruction(SILArgument*) = 0;
        virtual void parseInstruction(PartialApplyInst*) = 0;
    };

4. SILParser will implement these callbacks with the actual handling operations for each instruction.
    
    class SILParser: public SILParseInstruction
    {
    public:
        /*virtual*/void parseInstruction(SILArgument *r)
        {
            // Parse StringLiteralInst instruction
        }
        /*virtual*/void parseInstruction(PartialApplyInst *b)
        {
            // Parse PartialApplyInst instruction
        }
    };
    
5. The huge switch statement will be replaced with a single call:
  Opcode->parse(this);

What are your thoughts?

Sergey

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev

5

I agree with you completely.

Also, the SIL instruction parser does not actually have an instance of an instruction to perform virtual dispatch on. Nor should we introduce the concept of “wireframe” instructions just for the convenience of the parser. Redundancy between cases here should be addressed with normal redundancy elimination techniques, i.e. macros and templates.

John.

···

On Jan 13, 2016, at 3:25 PM, Jordan Rose via swift-dev <swift-dev@swift.org> wrote:
Hey, Sergey. It definitely makes sense to refactor this, but I don't think putting methods on SILInstruction is the way to go about it. There's no reason most clients of SIL need to know anything about parsing; from a separation-of-concerns perspective it belongs in a separate header and very likely a separate component (i.e. not lib/SIL/).

If this were Swift, we could use an extension, but alas. :-)

That said, I'm not a heavy user of SIL, so someone else from the Swift team with more of a stake should comment.

6

Hey, Sergey. It definitely makes sense to refactor this, but I don't think putting methods on SILInstruction is the way to go about it. There's no reason most clients of SIL need to know anything about parsing; from a separation-of-concerns perspective it belongs in a separate header and very likely a separate component (i.e. not lib/SIL/).

If this were Swift, we could use an extension, but alas. :-)

That said, I'm not a heavy user of SIL, so someone else from the Swift team with more of a stake should comment.

I agree with you completely.

Also, the SIL instruction parser does not actually have an instance of an instruction to perform virtual dispatch on. Nor should we introduce the concept of “wireframe” instructions just for the convenience of the parser. Redundancy between cases here should be addressed with normal redundancy elimination techniques, i.e. macros and templates.

The way to do this is to have a visitor parser that takes in a ValueKind and maps it to the parser routine to use.

Michael

···

On Jan 13, 2016, at 3:35 PM, John McCall via swift-dev <swift-dev@swift.org> wrote:

On Jan 13, 2016, at 3:25 PM, Jordan Rose via swift-dev <swift-dev@swift.org> wrote:

John.
_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev

7

Hey, Sergey. It definitely makes sense to refactor this, but I don't think putting methods on SILInstruction is the way to go about it. There's no reason most clients of SIL need to know anything about parsing; from a separation-of-concerns perspective it belongs in a separate header and very likely a separate component (i.e. not lib/SIL/).

If this were Swift, we could use an extension, but alas. :-)

That said, I'm not a heavy user of SIL, so someone else from the Swift team with more of a stake should comment.

I agree with you completely.

Also, the SIL instruction parser does not actually have an instance of an instruction to perform virtual dispatch on. Nor should we introduce the concept of “wireframe” instructions just for the convenience of the parser. Redundancy between cases here should be addressed with normal redundancy elimination techniques, i.e. macros and templates.

The way to do this is to have a visitor parser that takes in a ValueKind and maps it to the parser routine to use.

Let me rephrase. IIRC the way that this code is written is it first deserializes the value kind. Imagine if we had a visitor that was composed with the parser whose visitor methods would perform the relevant parsing.

···

On Jan 13, 2016, at 9:08 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:

On Jan 13, 2016, at 3:35 PM, John McCall via swift-dev <swift-dev@swift.org> wrote:

On Jan 13, 2016, at 3:25 PM, Jordan Rose via swift-dev <swift-dev@swift.org> wrote:

Michael

John.
_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

8

Michael,

The simplest way is to keep the switch statement in the parseSILInstruction method and move all logic into parser routines. Is it right?

bool SILParser::parseSILInstruction(SILBasicBlock *BB) {
  // Header

  switch (Opcode) {
  case ValueKind::SILArgument:
    return handleValueKindSILArgument(Opcode);
    break;
  case ValueKind::AllocBoxInst:
    return handleValueKindAllocBoxInst(Opcode);
    break;
  case ValueKind::ApplyInst:
    return handleValueKindApplyInst(Opcode);
    break;
  // ...
  }
}

SILParser::handleValueKindSILArgument(ValueKind Opcode) {
  // Handle ValueKind::SILArgument case here
}

SILParser::handleValueKindAllocBoxInst(ValueKind Opcode) {
  // Handle ValueKind::AllocBoxInst case here
}

SILParser::handleValueKindApplyInst(ValueKind Opcode) {
  // Handle ValueKind::ApplyInst case here
}

By the way, I’ve noticed that some instructions are getting parsed by parser routines already: parseSILFunctionRef or parseCallInstruction.

Sergey

···

Sent from my iPhone

On 14 Jan 2016, at 06:00, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:

On Jan 13, 2016, at 9:08 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:

On Jan 13, 2016, at 3:35 PM, John McCall via swift-dev <swift-dev@swift.org> wrote:

On Jan 13, 2016, at 3:25 PM, Jordan Rose via swift-dev <swift-dev@swift.org> wrote:
Hey, Sergey. It definitely makes sense to refactor this, but I don't think putting methods on SILInstruction is the way to go about it. There's no reason most clients of SIL need to know anything about parsing; from a separation-of-concerns perspective it belongs in a separate header and very likely a separate component (i.e. not lib/SIL/).

If this were Swift, we could use an extension, but alas. :-)

That said, I'm not a heavy user of SIL, so someone else from the Swift team with more of a stake should comment.

I agree with you completely.

Also, the SIL instruction parser does not actually have an instance of an instruction to perform virtual dispatch on. Nor should we introduce the concept of “wireframe” instructions just for the convenience of the parser. Redundancy between cases here should be addressed with normal redundancy elimination techniques, i.e. macros and templates.

The way to do this is to have a visitor parser that takes in a ValueKind and maps it to the parser routine to use.

Let me rephrase. IIRC the way that this code is written is it first deserializes the value kind. Imagine if we had a visitor that was composed with the parser whose visitor methods would perform the relevant parsing.

Michael

John.
_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev

9

No I mean like this:

1. Add a visitor in SILVisitor.h that just switches on value base.
2. Define a composition class with SILParser:

class SILInstructionParser {
   SILParser *P;

   ValueBase *visitValueBase() { llvm_unreachable("Unimplemented method"); }
   ValueBase *visitSILArgument.
};

...

SILValue SILInstructionParser::visitSILArgument() {
  ...
}

SILValue SILInstructionParser::visitAllocBoxInst() {
   ...
}

Michael

···

On Jan 14, 2016, at 9:00 AM, Sergey Bolshedvorsky <sergey@bolshedvorsky.com> wrote:

Michael,

The simplest way is to keep the switch statement in the parseSILInstruction method and move all logic into parser routines. Is it right?

bool SILParser::parseSILInstruction(SILBasicBlock *BB) {
  // Header

  switch (Opcode) {
  case ValueKind::SILArgument:
    return handleValueKindSILArgument(Opcode);
    break;
  case ValueKind::AllocBoxInst:
    return handleValueKindAllocBoxInst(Opcode);
    break;
  case ValueKind::ApplyInst:
    return handleValueKindApplyInst(Opcode);
    break;
  // ...
  }
}

SILParser::handleValueKindSILArgument(ValueKind Opcode) {
  // Handle ValueKind::SILArgument case here
}

SILParser::handleValueKindAllocBoxInst(ValueKind Opcode) {
  // Handle ValueKind::AllocBoxInst case here
}

SILParser::handleValueKindApplyInst(ValueKind Opcode) {
  // Handle ValueKind::ApplyInst case here
}

By the way, I’ve noticed that some instructions are getting parsed by parser routines already: parseSILFunctionRef or parseCallInstruction.

Sergey

Sent from my iPhone

On 14 Jan 2016, at 06:00, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Jan 13, 2016, at 9:08 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Jan 13, 2016, at 3:35 PM, John McCall via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Jan 13, 2016, at 3:25 PM, Jordan Rose via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
Hey, Sergey. It definitely makes sense to refactor this, but I don't think putting methods on SILInstruction is the way to go about it. There's no reason most clients of SIL need to know anything about parsing; from a separation-of-concerns perspective it belongs in a separate header and very likely a separate component (i.e. not lib/SIL/).

If this were Swift, we could use an extension, but alas. :-)

That said, I'm not a heavy user of SIL, so someone else from the Swift team with more of a stake should comment.

I agree with you completely.

Also, the SIL instruction parser does not actually have an instance of an instruction to perform virtual dispatch on. Nor should we introduce the concept of “wireframe” instructions just for the convenience of the parser. Redundancy between cases here should be addressed with normal redundancy elimination techniques, i.e. macros and templates.

The way to do this is to have a visitor parser that takes in a ValueKind and maps it to the parser routine to use.

Let me rephrase. IIRC the way that this code is written is it first deserializes the value kind. Imagine if we had a visitor that was composed with the parser whose visitor methods would perform the relevant parsing.

Michael

John.
_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

10

After few hours of research I think I have a clear picture what needs to be done.

The switch statement in bool SILParser::parseSILInstruction(SILBasicBlock *BB) will be replaced with few lines of code similar to these:

SILInstructionParser InstructionParser(*this);
ResultVal = InstructionParser.visit(Opcode);

There is going to be a new class with all visit methods:
namespace {
  class SILInstructionParser
    : public SILVisitor<SILInstructionParser, ValueBase> {
    public:
      SILParser &P;
      
      SILInstructionParser(SILParser &P): P(P) {}
      
// ValueBase visitSILArgument(ValueBase opcode) {
// llvm_unreachable("not an instruction");
// }
//
// ValueBase visitSILUndef(ValueBase opcode) {
// llvm_unreachable("not an instruction");
// }
    };
} // end anonymous namespace

And there is going to be a new method on SILVisitor, what will map opcode to the SILInstructionParser methods, something similar to:
  ValueRetTy visit(ValueBase *V) {
    switch (V->getKind()) {
#define VALUE(CLASS, PARENT) \
  case ValueKind::CLASS: \
    return asImpl().visit#class(static_cast<CLASS*>(V));
#include "swift/SIL/SILNodes.def"
    }
    llvm_unreachable("Not reachable, all cases handled");
  }

Is there a way how I can run only preprocessor, something similar to -E option for GCC?
I’m getting build error with my changes and I would like to see the output for the SILVisitor class.

Sergey

···

On 14 Jan 2016, at 19:27, Michael Gottesman <mgottesman@apple.com> wrote:

No I mean like this:

1. Add a visitor in SILVisitor.h that just switches on value base.
2. Define a composition class with SILParser:

class SILInstructionParser {
   SILParser *P;

   ValueBase *visitValueBase() { llvm_unreachable("Unimplemented method"); }
   ValueBase *visitSILArgument.
};

...

SILValue SILInstructionParser::visitSILArgument() {
  ...
}

SILValue SILInstructionParser::visitAllocBoxInst() {
   ...
}

Michael

On Jan 14, 2016, at 9:00 AM, Sergey Bolshedvorsky <sergey@bolshedvorsky.com <mailto:sergey@bolshedvorsky.com>> wrote:

Michael,

The simplest way is to keep the switch statement in the parseSILInstruction method and move all logic into parser routines. Is it right?

bool SILParser::parseSILInstruction(SILBasicBlock *BB) {
  // Header

  switch (Opcode) {
  case ValueKind::SILArgument:
    return handleValueKindSILArgument(Opcode);
    break;
  case ValueKind::AllocBoxInst:
    return handleValueKindAllocBoxInst(Opcode);
    break;
  case ValueKind::ApplyInst:
    return handleValueKindApplyInst(Opcode);
    break;
  // ...
  }
}

SILParser::handleValueKindSILArgument(ValueKind Opcode) {
  // Handle ValueKind::SILArgument case here
}

SILParser::handleValueKindAllocBoxInst(ValueKind Opcode) {
  // Handle ValueKind::AllocBoxInst case here
}

SILParser::handleValueKindApplyInst(ValueKind Opcode) {
  // Handle ValueKind::ApplyInst case here
}

By the way, I’ve noticed that some instructions are getting parsed by parser routines already: parseSILFunctionRef or parseCallInstruction.

Sergey

Sent from my iPhone

On 14 Jan 2016, at 06:00, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Jan 13, 2016, at 9:08 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Jan 13, 2016, at 3:35 PM, John McCall via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Jan 13, 2016, at 3:25 PM, Jordan Rose via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
Hey, Sergey. It definitely makes sense to refactor this, but I don't think putting methods on SILInstruction is the way to go about it. There's no reason most clients of SIL need to know anything about parsing; from a separation-of-concerns perspective it belongs in a separate header and very likely a separate component (i.e. not lib/SIL/).

If this were Swift, we could use an extension, but alas. :-)

That said, I'm not a heavy user of SIL, so someone else from the Swift team with more of a stake should comment.

I agree with you completely.

Also, the SIL instruction parser does not actually have an instance of an instruction to perform virtual dispatch on. Nor should we introduce the concept of “wireframe” instructions just for the convenience of the parser. Redundancy between cases here should be addressed with normal redundancy elimination techniques, i.e. macros and templates.

The way to do this is to have a visitor parser that takes in a ValueKind and maps it to the parser routine to use.

Let me rephrase. IIRC the way that this code is written is it first deserializes the value kind. Imagine if we had a visitor that was composed with the parser whose visitor methods would perform the relevant parsing.

Michael

John.
_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

11

After few hours of research I think I have a clear picture what needs to be done.

The switch statement in bool SILParser::parseSILInstruction(SILBasicBlock *BB) will be replaced with few lines of code similar to these:

SILInstructionParser InstructionParser(*this);
ResultVal = InstructionParser.visit(Opcode);

There is going to be a new class with all visit methods:
namespace {
  class SILInstructionParser
    : public SILVisitor<SILInstructionParser, ValueBase> {
    public:
      SILParser &P;
      
      SILInstructionParser(SILParser &P): P(P) {}
      
// ValueBase visitSILArgument(ValueBase opcode) {
// llvm_unreachable("not an instruction");
// }
//
// ValueBase visitSILUndef(ValueBase opcode) {
// llvm_unreachable("not an instruction");
// }
    };
} // end anonymous namespace

And there is going to be a new method on SILVisitor, what will map opcode to the SILInstructionParser methods, something similar to:
  ValueRetTy visit(ValueBase *V) {
    switch (V->getKind()) {
#define VALUE(CLASS, PARENT) \
  case ValueKind::CLASS: \
    return asImpl().visit#class(static_cast<CLASS*>(V));
#include "swift/SIL/SILNodes.def"
    }
    llvm_unreachable("Not reachable, all cases handled");
  }

Is there a way how I can run only preprocessor, something similar to -E option for GCC?

-E should work on any Unix-like C compiler.

Using an x-macro expansion like the above in the SILInstructionParser implementation makes sense; I don’t think there’s any point in adding it to SILVisitor, though.

John.

···

On Jan 17, 2016, at 9:02 AM, Sergey Bolshedvorsky via swift-dev <swift-dev@swift.org> wrote:

I’m getting build error with my changes and I would like to see the output for the SILVisitor class.

Sergey

On 14 Jan 2016, at 19:27, Michael Gottesman <mgottesman@apple.com <mailto:mgottesman@apple.com>> wrote:

No I mean like this:

1. Add a visitor in SILVisitor.h that just switches on value base.
2. Define a composition class with SILParser:

class SILInstructionParser {
   SILParser *P;

   ValueBase *visitValueBase() { llvm_unreachable("Unimplemented method"); }
   ValueBase *visitSILArgument.
};

...

SILValue SILInstructionParser::visitSILArgument() {
  ...
}

SILValue SILInstructionParser::visitAllocBoxInst() {
   ...
}

Michael

On Jan 14, 2016, at 9:00 AM, Sergey Bolshedvorsky <sergey@bolshedvorsky.com <mailto:sergey@bolshedvorsky.com>> wrote:

Michael,

The simplest way is to keep the switch statement in the parseSILInstruction method and move all logic into parser routines. Is it right?

bool SILParser::parseSILInstruction(SILBasicBlock *BB) {
  // Header

  switch (Opcode) {
  case ValueKind::SILArgument:
    return handleValueKindSILArgument(Opcode);
    break;
  case ValueKind::AllocBoxInst:
    return handleValueKindAllocBoxInst(Opcode);
    break;
  case ValueKind::ApplyInst:
    return handleValueKindApplyInst(Opcode);
    break;
  // ...
  }
}

SILParser::handleValueKindSILArgument(ValueKind Opcode) {
  // Handle ValueKind::SILArgument case here
}

SILParser::handleValueKindAllocBoxInst(ValueKind Opcode) {
  // Handle ValueKind::AllocBoxInst case here
}

SILParser::handleValueKindApplyInst(ValueKind Opcode) {
  // Handle ValueKind::ApplyInst case here
}

By the way, I’ve noticed that some instructions are getting parsed by parser routines already: parseSILFunctionRef or parseCallInstruction.

Sergey

Sent from my iPhone

On 14 Jan 2016, at 06:00, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Jan 13, 2016, at 9:08 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Jan 13, 2016, at 3:35 PM, John McCall via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Jan 13, 2016, at 3:25 PM, Jordan Rose via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
Hey, Sergey. It definitely makes sense to refactor this, but I don't think putting methods on SILInstruction is the way to go about it. There's no reason most clients of SIL need to know anything about parsing; from a separation-of-concerns perspective it belongs in a separate header and very likely a separate component (i.e. not lib/SIL/).

If this were Swift, we could use an extension, but alas. :-)

That said, I'm not a heavy user of SIL, so someone else from the Swift team with more of a stake should comment.

I agree with you completely.

Also, the SIL instruction parser does not actually have an instance of an instruction to perform virtual dispatch on. Nor should we introduce the concept of “wireframe” instructions just for the convenience of the parser. Redundancy between cases here should be addressed with normal redundancy elimination techniques, i.e. macros and templates.

The way to do this is to have a visitor parser that takes in a ValueKind and maps it to the parser routine to use.

Let me rephrase. IIRC the way that this code is written is it first deserializes the value kind. Imagine if we had a visitor that was composed with the parser whose visitor methods would perform the relevant parsing.

Michael

John.
_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev