[llvm-commits] BrainF compiler

[Chris Lattner]

On Sep 6, 2007, at 10:03 AM, S3 wrote:

>
> This is a new BrainF compiler to replace the old one.
> It uses the C++ API so it will generate code in the
> proper format.  It consolidates similar commands
> (blocks of ('+' and '-') and ('<' and '>')).
> It has an option for array bounds checking.

Nice!

> +#include "llvm/ADT/STLExtras.h"
> +#include "llvm/Analysis/Verifier.h"
> +#include "llvm/Bitcode/ReaderWriter.h"
> +#include "llvm/CallingConv.h"
> +#include "llvm/Constants.h"
> +#include "llvm/DerivedTypes.h"
> +#include "llvm/ExecutionEngine/GenericValue.h"
> +#include "llvm/ExecutionEngine/Interpreter.h"
> +#include "llvm/ExecutionEngine/JIT.h"
> +#include "llvm/Instructions.h"
> +#include "llvm/Module.h"
> +#include "llvm/ModuleProvider.h"
> +#include "llvm/Support/CommandLine.h"
> +#include "llvm/Support/ManagedStatic.h"
> +#include <fstream>
> +#include <iostream>
> +#include <cstdlib>

Please prune out include files you don't need.  There are at least a  
couple redundant or unreferenced ones.

> +cl::opt<CompileMode>
> +CompileMode("mode",
> +  cl::desc("Choose what to compile it to:"),
> +  cl::init(mode_LLAsm),
> +  cl::values(
> +       clEnumValN(mode_LLAsm, "ll",
> +                  "  Emit an LLVM assembly ('.ll') file"),
> +       clEnumValN(mode_BitCode, "bc",
> +                  "  Emit a bit code ('.bc') file"),
> +       clEnumValN(mode_JIT, "jit",
> +                  "  Run program with JIT compiling"),
> +       clEnumValEnd));

This is cute, but I'd suggest just having two modes to reduce link  
time.  For example, have JIT mode and .bc writer mode, but no .ll  
writer mode (or .ll but not .bc).  What do you think?


> +void BrainF::header() {
> +  module = new Module("BrainF");
> +
> +  //Function prototypes
> +
> +  //declare void @llvm.memset.i32(i8 *, i8, i32, i32)
> +  Function *memset_func;
> +  {
> +    const Type *memset_args[] = {
> +      PointerType::get(IntegerType::Int8Ty),
> +      IntegerType::Int8Ty,
> +      IntegerType::Int32Ty,
> +      IntegerType::Int32Ty
> +    };
> +    FunctionType *memset_type = FunctionType::
> +      get(Type::VoidTy,
> +          std::vector<const Type *>(memset_args, array_endof 
> (memset_args)),
> +          false, 0);
> +    memset_func = new Function(memset_type,  
> GlobalValue::ExternalLinkage,
> +                               "llvm.memset.i32", module);
> +    memset_func->setCallingConv(CallingConv::C);
> +  }

As a shortcut, you might consider calling module->getOrInsertFunction 
(PointerType::get(IntegerType::Int8Ty), IntegerType::Int8Ty,  
IntegerType::Int32Ty, IntegerType::Int32Ty, NULL);

There is no need to set the callingconv to C, it defaults to that.

> +  curbb = new BasicBlock(label, brainf_func);
> +
> +  //%arr = malloc i8, i32 %d
> +  ConstantInt *val_mem = ConstantInt::get(APInt(32, memtotal));
> +  ptr_arr = new MallocInst(IntegerType::Int8Ty, val_mem, "arr",  
> curbb);
> +
> +  //call void @llvm.memset.i32(i8 *%arr, i8 0, i32 %d, i32 1)
> +  {
> +    Value *memset_params[] = {
> +      ptr_arr,
> +      ConstantInt::get(APInt(8, 0)),
> +      val_mem,
> +      ConstantInt::get(APInt(32, 1))
> +    };
> +    CallInst *memset_call = new CallInst(memset_func, memset_params,
> +                                         array_endof(memset_params),
> +                                         "", curbb);
> +    memset_call->setCallingConv(CallingConv::C);
> +    memset_call->setTailCall(false);
> +  }

I'd strongly suggest using the LLVMBuilder (from "llvm/Support/ 
LLVMBuilder.h") interface to create code.  It makes the code much  
more terse to construct instructions.  Please ask if you have  
questions about how to use it.


> +  //Error block for array out of bounds
> +  if (comflag & flag_arraybounds)
> +  {
> +    //@aberrormsg = internal constant [%d x i8] c"\00"
> +    GlobalVariable *aberrormsg;
> +    {
> +      aberrormsg = new GlobalVariable(
> +        ArrayType::get(IntegerType::Int8Ty, 35),
> +        true,
> +        GlobalValue::InternalLinkage,
> +        0,
> +        "aberrormsg",
> +        module);
> +
> +      Constant *msg_0 = ConstantArray::
> +        get("Error: The head has left the tape.", true);
> +
> +      aberrormsg->setInitializer(msg_0);

This can be a little more self describing if you reorganize it like  
this:

> +      Constant *msg_0 = ConstantArray::
> +        get("Error: The head has left the tape.", true);

> +      aberrormsg = new GlobalVariable(msg_0->getType(),
> +        true,
> +        GlobalValue::InternalLinkage,
> +        msg_0,
> +        "aberrormsg",
> +        module);

Which then allows you to change the indentation.

>
> +        } }

>
> +  } } } }

Please don't pile up }'s like this, put them on their own line.  In  
cases where you have a lot of nesting like this, consider factoring  
the inner body out into a function call.  This can greatly reduce  
nesting and make the code much easier to follow.

This file contains two logically distinct pieces:

1. The code to parse the file and build the module.
2. The driver code that handles command line arguments, runs the jit,  
etc.

Please split this into two .cpp files.
>
> Index: examples/BrainF/Makefile
> ===================================================================
> --- examples/BrainF/Makefile	(revision 0)
> +++ examples/BrainF/Makefile	(revision 0)
> @@ -0,0 +1,15 @@
> +##===- examples/HowToUseJIT/Makefile -----------------------*-  
> Makefile -*-===##

Please update this first line.

-Chris