[PATCH] D101538: [GlobalISel][IRTranslator] Make translate() methods virtual.

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

In D101538#2758288 <https://reviews.llvm.org/D101538#2758288>, @zuban32 wrote:

> @foad hmm, with a naive approach I suppose we gonna run into the multiple inheritance problem. Let's assume we've created a base class as following:
>
>   template<typename T>
>   class BaseIRTranslator {
>   protected:
>     bool translate(const Instruction &Inst) {
>       return static_cast<T *>(this)->translateImpl(Inst);
>     }
>     bool translate(const Constant &C, Register Reg) {
>       return static_cast<T *>(this)->translateImpl(C, Reg);
>     }
>   };
>
> and then renamed old IRTranslator::translate methods to translateImpl. Then we have a target which wants to override them, but this new TargetIRTranslator would still have to be inherited from the original IRTranslator too as we want to use most of its functionality, so we end up with something like
>
>   class TargetIRTranslator : public IRTranslator, BaseIRTranslator<TargetIRTranslator> {
>   protected:
>     bool translateImpl(const Instruction &Inst);
>     bool translateImpl(const Constant &C, Register Reg);
>   }
>
> and translate() methods are defined twice. We used to have virtual inheritance for such kind of problems, but in this case it kills the whole idea of CRTP. Pls correct if I'm wrong

I haven't thought about it very hard, but I thinking of something like:

  template<typename T>
  class BaseIRTranslator {
  protected:
    bool translate(const Instruction &Inst) {
      return static_cast<T *>(this)->translateImpl(Inst);
    }
    bool translate(const Constant &C, Register Reg) {
      return static_cast<T *>(this)->translateImpl(C, Reg);
    }
    // ... the whole of the existing IRTranslator implementation goes here, with translate renamed to translateImpl ...
  };
  class IRTranslator : public BaseIRTranslator<IRTranslator> {
    // nothing needed here
  };
  class TargetIRTranslator : public BaseIRTranslator<TargetIRTranslator> {
    // override translateImpl here
  };



> Won't this problem go away when we introduce FP LLTs?

That would be even better :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101538/new/

https://reviews.llvm.org/D101538