Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More convenience functions for the IRBuilder #223

Merged
merged 11 commits into from Aug 29, 2018
Merged

Conversation

jmorag
Copy link
Contributor

@jmorag jmorag commented Aug 24, 2018

Provides convenience functions to generate global variables, global strings, and external varargs functions. Also provides the capability to easily check if the current block has a terminator and another instance for MonadModuleBuilder to allow functions with type
type Codegen = IRBuilderT (MonadModuleBuilderT (State SomeEnvironment)) to work as expected when passed as the body parameter of LLVM.IRBuilder.Module.function.

jmorag and others added 6 commits Aug 22, 2018
Works in the same vein as LLVM.IRBuilder.Module.extern and
LLVM.IRBuilder.Module.function.
It can be useful to have this when dealing with control flow to avoid
the possibility of trying to illegally branch twice inside of a basic
block.
Having
```instance MonadModuleBuilder m => MonadModuleBuilder (IRBuilderT m)```
allows for an easier transition from people using the old API as
described in the Kaleidescope tutorial to the current IRBuilder API. In
particular, if you try to do something like
```type LLVM = ModuleBuilderT (State LocalVars)
   type Codegen = IRBuilderT LLVM```
and you try to use the `@function` from IRBuilder.Module with a body
generator with type `Codegen ()`, this instance is necessary.
Copy link
Member

@cocreature cocreature left a comment

Looks great! A few minor details and then this should be ready to go.

@@ -262,6 +262,18 @@ currentBlock = liftIRState $ do
Just n -> pure n
Nothing -> error "Called currentBlock when no block was active"

-- | Find out if the currently active block has a terminator.
--
-- This function will fail under the same condition as @currentBlock
Copy link
Member

@cocreature cocreature Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an @ sign missing at the end.

@@ -209,5 +266,6 @@ instance (MonadModuleBuilder m, Monoid w) => MonadModuleBuilder (Lazy.RWST r w s
instance MonadModuleBuilder m => MonadModuleBuilder (StateT s m)
instance MonadModuleBuilder m => MonadModuleBuilder (Strict.StateT s m)
instance (Monoid w, MonadModuleBuilder m) => MonadModuleBuilder (Strict.WriterT w m)
instance (Monoid w, MonadModuleBuilder m) => MonadModuleBuilder (Lazy.WriterT w m)

-- Not an mtl instance, but necessary in order for @globalStringPtr to compile
Copy link
Member

@cocreature cocreature Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The terminating @ sign is also missing here.

pure $ ConstantOperand $ C.GlobalReference (ptr ty) nm

-- | Creates a series of instructions to generate a pointer to a string
-- constant. Useful for making format strings to pass to `printf`, for example
Copy link
Member

@cocreature cocreature Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to use @printf@ here, otherwise it will try to reference a Haskell identifier of that name.

emitDefn $ GlobalDefinition globalVariableDefaults
{ name = nm
, LLVM.AST.Global.type' = ty
, linkage = Weak
Copy link
Member

@cocreature cocreature Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation for using weak linkage here? I feel like External is the least surprising option and the one that we currently use for functions so we should only depart from that if there is a good reason for it (I’m definitely not saying that reason cannot exist!) and it should be documented.

emitDefn $ GlobalDefinition globalVariableDefaults
{ name = nm
, LLVM.AST.Global.type' = LLVM.AST.Typed.typeOf charArray
, linkage = Private
Copy link
Member

@cocreature cocreature Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question to the one for weak linkage: What is the reasoning behind making this Private?

Copy link
Contributor Author

@jmorag jmorag Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked what clang does for C global variables and string constants. For

#include <stdio.h>
int i;
float f;

int main()
{
    i = 1;
    printf("%d\n", i);

    f = 3.14;
    printf("%g\n", f);
    return 0;
}

clang emits i and f with common linkage and the strings with private linkage. Here's the full output, reformatted for clarity:

@i = common global i32 0, align 4
@.str = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
@f = common global float 0.000000e+00, align 4
@.str.1 = private unnamed_addr constant [4 x i8] c"%g\0A\00", align 1

define i32 @main() #0 {
  %1 = alloca i32, align 4
  store i32 0, i32* %1, align 4
  store i32 1, i32* @i, align 4
  %2 = load i32, i32* @i, align 4
  %3 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i32 %2)
  store float 0x40091EB860000000, float* @f, align 4
  %4 = load float, float* @f, align 4
  %5 = fpext float %4 to double
  %6 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i32 0, i32 0), double %5)
  ret i32 0
}
declare i32 @printf(i8*, ...) #1

Copy link
Contributor Author

@jmorag jmorag Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason that I had weak originally was that in the Kaleidescope tutorial, Stephen Diehl wrote something about global variables having weak linkage in the mutable variables chapter. However, changing the linkage doesn't seem to affect whether things compile or not. Up to you if you'd like everything to have uniform External linkage or conform to what clang does.

Copy link
Member

@cocreature cocreature Aug 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for string literals I think our situation is a bit different: In C string literals are anonymous so it doesn’t make sense to give them non-private linkage. But the globalStringPtr API explicitely requires a name. I would suggest that the API should either accept a name and use external linkage or produce a name automatically and make that private. I’m open to both but if you have no preference, then I would suggest we go with the former.

As for global variables, the difference in linkage is only relevant if you link multiple modules together. If you have weak linkage and you have two declarations of the same name, they will be merged. So if this is a global variable, it would be shared across modules. I don’t have particularly strong feelings here so it would be nice if @sdiehl could weigh in but generally I find external linkage to be less surprising so I’m slightly leaning toward that. Either way, it would probably be good to document the choice in the haddocks.

Copy link
Contributor Author

@jmorag jmorag Aug 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would maybe be possible to compromise for string literals; something like:

-- | Creates a series of instructions to generate a pointer to a string
-- constant. Useful for making format strings to pass to @printf@, for example
globalStringPtr
  :: (MonadModuleBuilder m, MonadIRBuilder m)
  => String       -- ^ The string to generate
  -> Maybe Name   -- ^ If a name is given, then the generated entity will be linked 
                  -- externally. If not, then a name will be chosen for it 
                  -- automatically and it will be linked privately.
  -> m Operand
globalStringPtr str nm = do
  let asciiVals = map (fromIntegral . ord) str
      llvmVals  = map (C.Int 8) (asciiVals ++ [0]) -- append null terminator
      char      = IntegerType 8
      charStar  = ptr char
      charArray = C.Array char llvmVals
  nm' <- maybe fresh return nm
  emitDefn $ GlobalDefinition globalVariableDefaults
    { name                  = nm'
    , LLVM.AST.Global.type' = LLVM.AST.Typed.typeOf charArray
    , linkage               = case nm of Nothing -> Private; Just _ -> External
    , isConstant            = True
    , initializer           = Just charArray
    , unnamedAddr           = Just GlobalAddr
    }
  pure $ ConstantOperand $ C.BitCast (C.GlobalReference charStar nm') charStar

could work, except that the MonadModuleBuilder class doesn't provide a convenient name generator. If you're OK with adding a MonadIRBuilder class constraint to the signature, then this would work, but otherwise, I think that going with external linkage is the easiest thing to do.

For global variables I have no preference whatsoever.

Copy link
Member

@sdiehl sdiehl Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the purpose of least surprises making External linkage the default seems sensible.

Copy link
Contributor Author

@jmorag jmorag Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Done in db3f39e.

@@ -262,6 +262,18 @@ currentBlock = liftIRState $ do
Just n -> pure n
Nothing -> error "Called currentBlock when no block was active"

-- | Find out if the currently active block has a terminator.
--
-- This function will fail under the same condition as @currentBlock@
Copy link
Contributor Author

@jmorag jmorag Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. I've clearly never written haddocks before.

@cocreature cocreature merged commit 18588d3 into llvm-hs:llvm-6 Aug 29, 2018
1 check passed
@cocreature
Copy link
Member

cocreature commented Aug 29, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants