[llvm-commits] Proposed patch for intrinsic parameter type matching

[Chris Lattner]

On Jul 31, 2007, at 11:18 AM, Chandler Carruth wrote:
> This is a patch that I have reworked based on Dan Gohman's original
> patch to be more incremental and address several issues that were  
> raised
> on the -commits list. This does change the Intrinsics.td format and  
> the
> mechanisms for defining new intrinsic functions, and so I would  
> like to
> get some feedback on it before committing it.

> Index: include/llvm/AutoUpgrade.h
> ===================================================================
> --- include/llvm/AutoUpgrade.h	(revision 0)
> +++ include/llvm/AutoUpgrade.h	(revision 0)
> @@ -0,0 +1,43 @@
> +//===-- llvm/Assembly/AutoUpgrade.h - AutoUpgrade Helpers -------- 
> *- C++ -*-===//

The header is not quite correct (llvm/Assembly -> llvm), and is one  
char too long :)

> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file was developed by Reid Spencer is distributed under  
> the University

s/Reid/Chandler/

> +// of Illinois Open Source License. See LICENSE.TXT for details.
> +//
> +// 
> ===------------------------------------------------------------------- 
> ---===//
> +//
> +//  These functions are implemented by the lib/VMCore/ 
> AutoUpgrade.cpp.

s/the//

> +//
> +// 
> ===------------------------------------------------------------------- 
> ---===//
> +
> +#ifndef LLVM_ASSEMBLY_AUTOUPGRADE_H
> +#define LLVM_ASSEMBLY_AUTOUPGRADE_H

s/ASSEMBLY_//

> +
> +#include <string>
> +#include <vector>

These two #includes can be removed.

> +namespace llvm {
> +  class Function;
> +  class CallInst;
> +  class Instruction;
> +  class Value;
> +  class BasicBlock;

It doesn't look like Value or Instruction are needed.


> Index: lib/VMCore/AutoUpgrade.cpp
> ===================================================================
> --- lib/VMCore/AutoUpgrade.cpp	(revision 0)
> +++ lib/VMCore/AutoUpgrade.cpp	(revision 0)
> @@ -0,0 +1,243 @@
> +//===-- AutoUpgrade.cpp - Implement auto-upgrade helper functions  
> ---------===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file was developed by Reid Spencer and is distributed  
> under the

s/Reid/Chandler/

> +// University of Illinois Open Source License. See LICENSE.TXT for  
> details.
> +//
> +// 
> ===------------------------------------------------------------------- 
> ---===//
> +//
> +// This file implements the auto-upgrade helper functions
> +//
> +// 
> ===------------------------------------------------------------------- 
> ---===//
> +
> +#include "llvm/AutoUpgrade.h"
> +#include "llvm/Constants.h"
> +#include "llvm/DerivedTypes.h"
> +#include "llvm/Function.h"
> +#include "llvm/Module.h"
> +#include "llvm/ValueSymbolTable.h"
> +#include "llvm/Instructions.h"
> +#include "llvm/ParameterAttributes.h"
> +#include "llvm/Intrinsics.h"

Please see if you can prune down this list at all.

> +using namespace llvm;
> +
> +
> +Function* llvm::UpgradeIntrinsicFunction(Function *F) {
> +  assert(F && "Illegal to upgrade a non-existent Function.");
> +
> +  // Get the Function's name.
> +  const std::string& Name = F->getName();
> +
> +  // Convenience
> +  const FunctionType *FTy = F->getFunctionType();
> +
> +  // Quickly eliminate it, if it's not a candidate.
> +  if (Name.length() <= 8 || Name[0] != 'l' || Name[1] != 'l' ||
> +      Name[2] != 'v' || Name[3] != 'm' || Name[4] != '.')
> +    return 0;
> +
> +  Module *M = F->getParent();
> +  switch (Name[5]) {
> +  default: break;
> +  case 'b':
> +    //  We only care about 'llvm.bswap.i*.i*', so check for  
> 'bswap.' and then
> +    //  for there being a '.' after 'bswap.'

Please describe what this does.  For example, something like this  
would be sufficient:

// Upgrade llvm.bswap.i32.i32 -> llvm.bswap.i32.

Likewise for the others.

> +    if (Name.compare(5,6,"bswap.",6) == 0) {
> +      std::string::size_type delim = Name.find('.',11);
> +
> +      if (delim != std::string::npos) {
> +        //  Construct a new name as 'llvm.bswap' + '.i*'
> +        std::string NewName = Name.substr(0,10)+Name.substr(delim);
> +        return cast<Function>(M->getOrInsertFunction(NewName,
> +                                                     F- 
> >getReturnType(),
> +                                                     F- 
> >getReturnType(),
> +                                                     (Type *)0));

Instead of returning a new function, why not just change the name of  
the existing one?  You're not changing the type of the arguments or  
result here.  This would be more efficient.

Likewise for part.select and part.set.

> +  case 'c':
> +    //  We only want to fix intrinsics which do not have the  
> correct return
> +    //  type, so we check for the name 'llvm.ct*', and then check  
> if the return
> +    //  type does not match the parameter type.
> +    if ( (Name.compare(5,5,"ctpop",5) == 0 ||
> +          Name.compare(5,4,"ctlz",4) == 0 ||
> +          Name.compare(5,4,"cttz",4) == 0) &&
> +        FTy->getReturnType() != FTy->getParamType(0)) {
> +      ValueSymbolTable &SymTab = M->getValueSymbolTable();
> +
> +      //  We first need to change the name of the old (bad)  
> intrinsic, because
> +      //  its type is incorrect, but we cannot overload that name. We
> +      //  arbitrarily unique it here allowing us to construct a  
> correctly named
> +      //  and typed function below.
> +      F->setName(SymTab.getUniqueName(F->getName()));

Instead of doing this, just use F->setName("");

> +
> +      //  Now construct the new intrinsic with the correct name  
> and type. We
> +      //  leave the old function around in order to query its  
> type, whatever it
> +      //  may be, and correctly convert up to the new type.
> +      return cast<Function>(M->getOrInsertFunction(Name,
> +                                                   FTy- 
> >getParamType(0),
> +                                                   FTy- 
> >getParamType(0),
> +                                                   (Type *)0));

Makes sense.

> +// CastArg - Perform the appropriate cast of an upgraded argument.
> +static Value *CastArg(Value *Arg, bool SrcSExt,
> +                      const Type *Ty, bool DestSExt,
> +                      Instruction *InsertBefore) {
> +  Instruction::CastOps    op  = CastInst::getCastOpcode(Arg,  
> SrcSExt, Ty, DestSExt);
> +
> +  if (Constant *C = dyn_cast<Constant>(Arg)) {
> +    return ConstantExpr::getCast(op, C, Ty);
> +  } else {
> +    return CastInst::create(op, Arg, Ty, "autoupgrade_cast",
> +                            InsertBefore);
> +  }
> +}

No intrinsics need arguments to be upgraded.  Plz remove this or  
rename it.

> +// UpgradeIntrinsicCall - Change a call to an old intrinsic to be  
> a call to an
> +// upgraded intrinsic.
> +void llvm::UpgradeIntrinsicCall(CallInst *CI, Function *NewFn) {
> +  assert(NewFn && "Cannot upgrade an intrinsic call without a new  
> function.");

This isn't your fault, but:

I don't like way this function is implemented.  The problem is that  
it tries to be a general solution for coercing an old function to a  
new one.

Instead, what do you think about something like this:

switch (NewFn->getIntrinsicID()) {
default:  assert(0 && "Unknown function to upgrade");
case ctpop, ctlz,cttz:
   // Okay, we know that the result type changed.  Do exactly the  
upgrade needed for these operations.
   break;
}

This makes it much easier to convince ourselves that this is correct,  
because it's not trying to be a very general solution.  The code will  
also be much much simpler.


Other than that, the patch looks very nice.  Please address the  
issues above, retest, then commit it.  If there are any other issues  
I'll pick them up on the commit.

Thanks for tackling this Chandler!

-Chris