llvm dev - May 2013 - [LLVMdev] Types vs. register classes in instruction patterns -- effect on FastISel

Hello,

In http://llvm.org/viewvc/llvm-project?view=revision&revision=177889 and
http://llvm.org/viewvc/llvm-project?view=revision&revision=177890 (along
with some follow-up patches) the PowerPC back end was changed to use
types instead of register classes in instruction patterns.  This matched
similar changes that Jakob made for Sparc in r177835.

I've recently come across an unpleasant side-effect of this change.  I
started looking at implementing FastISel for PPC64, and discovered that
practically all of our patterns were rejected when looking for simple
patterns eligible for FastEmit_*.  Stepping through the tablegen code
showed that we were rejecting them precisely because the patterns now
contain value types instead of register classes (FastISelEmitter.cpp,
lines 253ff.):

      // For now, the only other thing we accept is register operands.
      const CodeGenRegisterClass *RC = 0;
      if (OpLeafRec->isSubClassOf("RegisterOperand"))
        OpLeafRec = OpLeafRec->getValueAsDef("RegClass");
      if (OpLeafRec->isSubClassOf("RegisterClass"))
        RC = &Target.getRegisterClass(OpLeafRec);
      else if (OpLeafRec->isSubClassOf("Register"))
        RC = Target.getRegBank().getRegClassForRegister(OpLeafRec);
      else
        return false;

What's the proper thing to do here?  If we can map a ValueType to a
register class, then we'd be ok.  I don't immediately see an interface
that will do this (indeed, I guess I'm not certain that's guaranteed to
be a 1-1 mapping).

This wasn't noticed previously on Sparc or PowerPC since neither has had
a FastISel implementation yet.  Thoughts?

Thanks,
Bill

On May 17, 2013, at 2:04 PM, Bill Schmidt <wschmidt at linux.vnet.ibm.com>
wrote:
> Hello,
> 
> In http://llvm.org/viewvc/llvm-project?view=revision&revision=177889
and
> http://llvm.org/viewvc/llvm-project?view=revision&revision=177890
(along
> with some follow-up patches) the PowerPC back end was changed to use
> types instead of register classes in instruction patterns.  This matched
> similar changes that Jakob made for Sparc in r177835.
> 
> I've recently come across an unpleasant side-effect of this change.  I
> started looking at implementing FastISel for PPC64, and discovered that
> practically all of our patterns were rejected when looking for simple
> patterns eligible for FastEmit_*.  Stepping through the tablegen code
> showed that we were rejecting them precisely because the patterns now
> contain value types instead of register classes (FastISelEmitter.cpp,
> lines 253ff.):
> 
>      // For now, the only other thing we accept is register operands.
>      const CodeGenRegisterClass *RC = 0;
>      if (OpLeafRec->isSubClassOf("RegisterOperand"))
>        OpLeafRec = OpLeafRec->getValueAsDef("RegClass");
>      if (OpLeafRec->isSubClassOf("RegisterClass"))
>        RC = &Target.getRegisterClass(OpLeafRec);
>      else if (OpLeafRec->isSubClassOf("Register"))
>        RC = Target.getRegBank().getRegClassForRegister(OpLeafRec);
>      else
>        return false;
> 
> What's the proper thing to do here?  If we can map a ValueType to a
> register class, then we'd be ok.  I don't immediately see an
interface
> that will do this (indeed, I guess I'm not certain that's
guaranteed to
> be a 1-1 mapping).
> 
> This wasn't noticed previously on Sparc or PowerPC since neither has
had
> a FastISel implementation yet.  Thoughts?
That’s a lot of ‘for nows’ in one function...

It seems to me that the code is looking at the wrong pattern to find register
classes. The input patterns match on types, the instructions in the output
patterns specify register classes.

However, the register class isn’t really used for anything, so maybe the code
should simply be expanded to also accept patterns where all the input operands
have the same type.

/jakob

On Fri, 2013-05-17 at 15:32 -0700, Jakob Stoklund Olesen
wrote:
> On May 17, 2013, at 2:04 PM, Bill Schmidt <wschmidt at
linux.vnet.ibm.com> wrote:
> 
> > Hello,
> > 
> > In
http://llvm.org/viewvc/llvm-project?view=revision&revision=177889 and
> > http://llvm.org/viewvc/llvm-project?view=revision&revision=177890
(along
> > with some follow-up patches) the PowerPC back end was changed to use
> > types instead of register classes in instruction patterns.  This
matched
> > similar changes that Jakob made for Sparc in r177835.
> > 
> > I've recently come across an unpleasant side-effect of this
change.  I
> > started looking at implementing FastISel for PPC64, and discovered
that
> > practically all of our patterns were rejected when looking for simple
> > patterns eligible for FastEmit_*.  Stepping through the tablegen code
> > showed that we were rejecting them precisely because the patterns now
> > contain value types instead of register classes (FastISelEmitter.cpp,
> > lines 253ff.):
> > 
> >      // For now, the only other thing we accept is register operands.
> >      const CodeGenRegisterClass *RC = 0;
> >      if (OpLeafRec->isSubClassOf("RegisterOperand"))
> >        OpLeafRec = OpLeafRec->getValueAsDef("RegClass");
> >      if (OpLeafRec->isSubClassOf("RegisterClass"))
> >        RC = &Target.getRegisterClass(OpLeafRec);
> >      else if (OpLeafRec->isSubClassOf("Register"))
> >        RC = Target.getRegBank().getRegClassForRegister(OpLeafRec);
> >      else
> >        return false;
> > 
> > What's the proper thing to do here?  If we can map a ValueType to
a
> > register class, then we'd be ok.  I don't immediately see an
interface
> > that will do this (indeed, I guess I'm not certain that's
guaranteed to
> > be a 1-1 mapping).
> > 
> > This wasn't noticed previously on Sparc or PowerPC since neither
has had
> > a FastISel implementation yet.  Thoughts?
> 
> That’s a lot of ‘for nows’ in one function...
> 
> It seems to me that the code is looking at the wrong pattern to find
register classes. The input patterns match on types, the instructions in the
output patterns specify register classes.
> 
> However, the register class isn’t really used for anything, so maybe the
code should simply be expanded to also accept patterns where all the input
operands have the same type.
Well, the specific register class isn't used, but the fact that
something register-like was found is used downstream.  For grins I tried
eliminating the code to check for register-ness and found that both the
X86 and ARM targets failed to build.  The .inc file was built for X86,
but the generated code was nonsensical for certain kinds of patterns
(function names containing .getPtrType() as a substring, etc.).  ARM
couldn't build the .inc file at all due to duplicate patterns.

So I think we have to specifically recognize ValueTypes.  To keep things
simple, we could pass in the destination register class from
collectPatterns, which has the output register class of the destination
pattern, and only permit a ValueType representable by that RC.

For an experiment, I tried allowing all ValueType source operands.  This
had no effect on code generated for the ARM and X86 targets, so that's
good.  However, I ran into problems with duplicates generated into the
FastISel tables for PowerPC, due to the multiclass definitions for
recording forms.  So there's still work to be done there.

Thanks,
Bill
> 
> /jakob
>