[GlobalISel][IRTranslator] Make translate() methods virtual.

[zuban32]

Summary

Although IRTranslator::translate() methods were designed as
general as possible, for some targets (e.g. SPIR-V) it's
quite convenient to override those to be able to perform
some very target-specific activities.

[zuban32]

Rebased

[zuban32]

Gentle ping

[aemerson]

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

[zuban32]

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

[aemerson]

In D101538#2750853, @zuban32 wrote:

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

I see. Is this a widespread issue with a lot of the translation functions or just bitcasts? If it's just bitcast you could just make that virtual so we don't have to pay the virtual function call overhead every translate. If not, then I'm ok with doing this.

[zuban32]

In D101538#2751818, @aemerson wrote:

In D101538#2750853, @zuban32 wrote:

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

I see. Is this a widespread issue with a lot of the translation functions or just bitcasts? If it's just bitcast you could just make that virtual so we don't have to pay the virtual function call overhead every translate. If not, then I'm ok with doing this.

Unfortunately not, we have custom lowering for GEPs, extract/insertvalues,, a couple of others too. The set itself is not that big, but IMO it'd look weird if 8-10 translateXXX functions would be virtual while others are not.

[aemerson]

Fair enough.

[foad]

In D101538#2751818, @aemerson wrote:

In D101538#2750853, @zuban32 wrote:

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

I see. Is this a widespread issue with a lot of the translation functions or just bitcasts? If it's just bitcast you could just make that virtual so we don't have to pay the virtual function call overhead every translate. If not, then I'm ok with doing this.

How about doing it with https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern#Static_polymorphism to avoid the virtual function call overhead?

[zuban32]

In D101538#2753323, @foad wrote:

In D101538#2751818, @aemerson wrote:

In D101538#2750853, @zuban32 wrote:

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

I see. Is this a widespread issue with a lot of the translation functions or just bitcasts? If it's just bitcast you could just make that virtual so we don't have to pay the virtual function call overhead every translate. If not, then I'm ok with doing this.

How about doing it with https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern#Static_polymorphism to avoid the virtual function call overhead?

@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

[zuban32]

@aemerson @foad if no one has any idea how to make CRTP approach feasible here, then I'd suggest to merge this, I have no write access though

[arsenm]

In D101538#2751818, @aemerson wrote:

In D101538#2750853, @zuban32 wrote:

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

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

[foad]

In 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 :-)

[zuban32]

In D101538#2764529, @arsenm wrote:

In D101538#2751818, @aemerson wrote:

In D101538#2750853, @zuban32 wrote:

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

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

I wouldn't be so sure about that. One particular usecase I need this for is a bitcast between pointer types:

%1 = bitcast i8* %0 to %struct.ST*

In our target they're both of p0 LLTs, so the default method eliminates this instruction while in our target it's necessary to keep it.

[foad]

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

I wouldn't be so sure about that. One particular usecase I need this for is a bitcast between pointer types:

%1 = bitcast i8* %0 to %struct.ST*

In our target they're both of p0 LLTs, so the default method eliminates this instruction while in our target it's necessary to keep it.

What are you going to do when IR switches to opaque pointers? https://llvm.org/docs/OpaquePointers.html

[zuban32]

In D101538#2768105, @foad wrote:

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

I wouldn't be so sure about that. One particular usecase I need this for is a bitcast between pointer types:

%1 = bitcast i8* %0 to %struct.ST*

In our target they're both of p0 LLTs, so the default method eliminates this instruction while in our target it's necessary to keep it.

What are you going to do when IR switches to opaque pointers? https://llvm.org/docs/OpaquePointers.html

Good point, an honest answer would be "We haven't thought about it yet" :) BTW is there any exact timeline of the switch? I saw some related patches sent, but not sure when that's going to happen

[qcolombet]

I wouldn't be so sure about that. One particular usecase I need this for is a bitcast between pointer types:

%1 = bitcast i8* %0 to %struct.ST*
In our target they're both of p0 LLTs, so the default method eliminates this instruction while in our target it's necessary to keep it.

That sounds like you're abusing the IR here. Shouldn't you have a different addrspace here?

FWIW, when we discussed the design of GISel internally originally we explicitly chose not to allow virtual overloading of the IR translation steps because that step follow what is represented in the IR.
To make a parallel with SDISel, this is as if we would allow SDISelBuilder to be target specific.

Bottom line, for just this one use case I feel that it doesn't make sense to allow virtual method everywhere yet.

You could workaround that with introducing a pseudo/intrisinc in codegen prepare for instance.

My 2 cents.

Cheers,
-Quentin

[zuban32]

In D101538#2769796, @qcolombet wrote:

I wouldn't be so sure about that. One particular usecase I need this for is a bitcast between pointer types:

%1 = bitcast i8* %0 to %struct.ST*
In our target they're both of p0 LLTs, so the default method eliminates this instruction while in our target it's necessary to keep it.

That sounds like you're abusing the IR here. Shouldn't you have a different addrspace here?

FWIW, when we discussed the design of GISel internally originally we explicitly chose not to allow virtual overloading of the IR translation steps because that step follow what is represented in the IR.
To make a parallel with SDISel, this is as if we would allow SDISelBuilder to be target specific.

Bottom line, for just this one use case I feel that it doesn't make sense to allow virtual method everywhere yet.

You could workaround that with introducing a pseudo/intrisinc in codegen prepare for instance.

My 2 cents.

Cheers,
-Quentin

OK, thank you Quentin, Jay and Matt for your input, I understand it's kind of a bruteforce approach to make these function virtual, I'll try to consider another viable options

[zuban32]

@qcolombet @arsenm @foad this change is already abandoned, but my question seems directly related to it.

I have the following problem - I want to generate a target instruction which denotes LLVM type of a value and its argument should be the value's vreg. Correct me if I'm wrong, the only stage I can do that at is IRTranslator since later we don't have any access to LLVM values at all, but IRTranslator's VMap is private and no other way to access such info exists. And we've already discussed here that overriding translate() methods is not good enough, what other ways of coping with that can you suggest?

I thought about extending GISelObserver with something like 'VRegCreated(Value*,VReg), what do you think? Or perhaps some TTI hook called within translate/getOrCreateVRegs.

I've tried approaches with target pseudo intrinsics and extra IR passes, but still everything depends on missing the external (relatively to the IRTranslator) way to obtain the info about Value->Vreg mapping.

[qcolombet]

Hey!

I've tried approaches with target pseudo intrinsics and extra IR passes, but still everything depends on missing the external (relatively to the IRTranslator) way to obtain the info about Value->Vreg mapping.

That sounds similar to what DBG_VALUE does (from my 10,000 feet view of it :)) and this works with IR intrinsics.
What is missing for this kind of approaches to work in your case?

Cheers,
-Quentin

[zuban32]

Let me explain my problem with a short example. Assume we have the following IR:

define @foo(i32* addrspace(3) %v.addr) {
%0 = load i32 i32* addrspace(3) %v.addr
...
}

In our target this type info will require 2 additional instructions, so after instruction selection the code should look like:

r0 = OpIntegerType 32         // op0: integer width
r1 = OpPointerType r0, 3      // op0: base type, op1: addrspace
...
r2 = OpParam r1     // op0: the actual type of the func's argument
r3 = OpLoad r0, r2  // op0: 'ResultType', op1: the actual operand

In order to generate the type instructions we need the original LLVM type info, but also it's required to keep the mapping of these types to the vregs generated by the IRTranslator so that this 'ResultType' field would be correctly set at the later GISel stages.
But if we don't intervene into the IR translation process, I don't see how it is possible to obtain this mapping.
Speaking about DBG_VALUE - I'm not quite familiar with it, but FWIW it seems that it uses some metadata nodes as its operand, is it correct?

llvm/include/llvm/CodeGen/GlobalISel/IRTranslator.h

private:
   ///   CSE may not catch them.
   ///   => Update ValToVReg and remember that we saw a constant in Constants.
   ///   We will materialize all the constants in finalize.
   /// Note: we would need to do something so that we can recognize such operand
   ///       as constants.
   /// 3. Create the generic instruction.
   ///
   /// \return true if the translation succeeded.
-  bool translate(const Instruction &Inst);
+  virtual bool translate(const Instruction &Inst);
 
   /// Materialize \p C into virtual-register \p Reg. The generic instructions
   /// performing this materialization will be inserted into the entry block of
   /// the function.
   ///
   /// \return true if the materialization succeeded.
-  bool translate(const Constant &C, Register Reg);
+  virtual bool translate(const Constant &C, Register Reg);
 
   // Translate U as a copy of V.
   bool translateCopy(const User &U, const Value &V,
                      MachineIRBuilder &MIRBuilder);
 
   /// Translate an LLVM bitcast into generic IR. Either a COPY or a G_BITCAST is
   /// emitted.
   bool translateBitCast(const User &U, MachineIRBuilder &MIRBuilder);