[llvm-commits] [PATCH] Analog Devices Blackfin back-end

[Jakob Stoklund Olesen]

On 29/07/2009, at 08.14, Chris Lattner wrote:
> Cool, the autoconf patch is fine, feel free to add it now if it  
> won't break anything.

I think it will break the build if it goes in before the Target/ 
Blackfin directory is created.

> A "cumulative" patch is preferred, but the delta wasn't that big and  
> I'm not trying to do a final review, just give some tips.

I will fix these issues and then submit a full patch.

> Please put the name and return type on the same line if there is  
> room, e.g.:
>
> +SDNode *BlackfinDAGToDAGISel::Select(SDValue Op) {
>
> Please use K&R style bracketing, where the { goes on the same line  
> as the function name.

No problem. I will use the same style as the rest of lib/Target.

BTW, that's not K&R style: http://en.wikipedia.org/wiki/Indent_style#K.26R_style

> +  if (MI->getOperand(opNum+1).isImm()
> +    && MI->getOperand(opNum+1).getImm() == 0)
> +    return;
>
> Please use && on the previous line, like so:
>
> +  if (MI->getOperand(opNum+1).isImm() &&
> +      MI->getOperand(opNum+1).getImm() == 0)
> +    return;

Much better.

> Please consistently indent by 2:
>
> +    default: return true;  // Unknown modifier.
> +    case 'r':
> +     break;
> +    }

Heh, that is copied verbatim from the Sparc target. Will fix.

> Please just use Bfin:: in isCC for consistency:
>
> +static inline bool
> +isCC(const TargetRegisterClass *RC)
> +{
> +  using namespace Bfin;
> +  return RC == &AnyCCRegClass || AnyCCRegClass.hasSubClass(RC);
> +}
> +
> +static inline bool
> +isDCC(const TargetRegisterClass *RC)
> +{
> +  return RC == &Bfin::DRegClass || Bfin::DRegClass.hasSubClass(RC)  
> || isCC(RC);
> +}

Will fix.

> On that note, the Bfin target name space is kind of a weird  
> contraction for it, I don't have a strong opinion on it either way,  
> but maybe BF would be better?  Your call, do whatever you like.

All the other targets use an all-caps abbreviation, so that is  
probably a good idea. The "bfin" contraction comes from GCC: - 
march=bfin and predefined __bfin__ macro.

[...]
> How about using early exits, giving something like:
>
> +  if (inClass(Bfin::DPRegClass, SrcReg, RC)) {
> +    BuildMI(MBB, I, DL, get(Bfin::STORE32fi))
> +      .addReg(SrcReg, getKillRegState(isKill))
> +      .addFrameIndex(FI)
> +      .addImm(0);
>     return;
> +  }

Will fix.

> It seems that bfin has a number of common "kinds" of instruction,  
> could these benefit from a multipattern to factor them?

Yes, maybe. The instruction set is oddly irregular so you often get  
only two instructions per multipattern. But I haven't really looked  
for these opportunities, I will see what I can find.

Thanks,
/jakob