On Feb 8, 2009, at 8:58 AM, Dan Gohman wrote:> Hi Chris, > > Would it be better to split add into multiple opcodes instead of using > SubclassData bits?No, I don't think so. The big difference here is that (like type) "opcode" never changes for an instruction once it is created. I expect that optimizations would want to play with these (e.g. convert them to 'undefined' when it can prove overflow never happens) so I think it is nice to not have it be in the opcode field. This also interacts with FP rounding mode stuff, which I expect to handle the same way with FP operations some day.> Compare this: > > switch (I->getOpcode()) { > case Instruction::Add: { > switch (cast<Add>(I)->getOverflowBehavior()) { > case AddInstruction::Wrapping: > // ... > case AddInstruction::UndefinedSigned: > // ... > case AddInstruction::UndefinedUnsigned:Sure, that is ugly. However, I think it would be much more common to look at these in "isa" flavored tests than switches: if (isa<SAdd_OpenInst>(X)) is much nicer than: if (BinaryOperator *BO = dyn_cast<BinaryOperator>(x)) if (BO->getOpcode() == blah::Add && BO->getOverflow() == blah::Undefined) However, we a) already suffer this just for Add, because we don't have an AddInst class, and b) don't care about the opcode anyway. IntrinsicInst is a good example of how we don't actually need opcode bits or concrete classes to make isa "work". It would be a nice cleanup to add new "pseudo instruction" classes like IntrinsicInst for all the arithmetic anyway. If the switch case really does become important, we can just add a getOpcodeWithSubtypes() method that returns a new flattened enum.>-Chris
On Feb 8, 2009, at 11:33 AM, Chris Lattner wrote:> > On Feb 8, 2009, at 8:58 AM, Dan Gohman wrote: > >> Hi Chris, >> >> Would it be better to split add into multiple opcodes instead of >> using >> SubclassData bits? > > No, I don't think so. The big difference here is that (like type) > "opcode" never changes for an instruction once it is created. I > expect that optimizations would want to play with these (e.g. convert > them to 'undefined' when it can prove overflow never happens) so I > think it is nice to not have it be in the opcode field.Why is this? If SubclassData can be modified, why not SubclassID too? Having it const may help guard against something accidentally changing it to an opcode that would require a different subclass, but it's a private member, so modifications to it could be fairly effectively controlled. I agree that isa/dyn_cast can be quite flexible, but they can handle ranges of opcodes just as well as they can handle opcodes composed from multiple fields. The big-switch idiom is a staple of compiler construction; it would be nice to be able to continue to use it directly. Dan
On Feb 9, 2009, at 8:20 AM, Dan Gohman wrote:> > On Feb 8, 2009, at 11:33 AM, Chris Lattner wrote: > >> >> On Feb 8, 2009, at 8:58 AM, Dan Gohman wrote: >> >>> Hi Chris, >>> >>> Would it be better to split add into multiple opcodes instead of >>> using >>> SubclassData bits? >> >> No, I don't think so. The big difference here is that (like type) >> "opcode" never changes for an instruction once it is created. I >> expect that optimizations would want to play with these (e.g. convert >> them to 'undefined' when it can prove overflow never happens) so I >> think it is nice to not have it be in the opcode field. > > Why is this? If SubclassData can be modified, why not SubclassID too? > Having it const may help guard against something accidentally changing > it to an opcode that would require a different subclass, but it's a > private > member, so modifications to it could be fairly effectively controlled.There is no technical reason, it just provides a more clear API and makes it easier to reason about. For example since SubclassID can't change, you don't have issues where you'd need to change the actual class of the impl. -Chris