Add ConstantAggregateZero ConstantExpr and a few more methods #71
Conversation
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Adds DataLayout.getTypeAllocSize which obtains the size of a given LVLM Type as a number. Useful for passing to things like malloc() Signed-off-by: Vihan <[email protected]>
Returns type size in bits. Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
There was a problem hiding this comment.
Hi @vihanb
Thank you for your efforts!
Personally, I prefer to have smaller PRs since those are simpler to review and normally faster merged. But I understand that you want to avoid merge conflicts whenever possible.
I tried to summarize your changes:
- Adds ConstantAggregateZero
- Adds ConstantExpr
- Adds isFloatingPointTy to Type
- Adds CreateFPToUI, CreateIsNull, CreateZExtOrTrunc, and CreateSExtOrTrunc to IRBuilder
Is this summary correct?
I added some smaller review comments. Most notably is that the typing definition file is not up to date with your made changes. There also seems to be a merging issue since the tests & build is failing for some reason.
I further ask you to please write a unit test for each added method that ensures that the function can at least be called (and best assert something). I know you tested it in your project. But the project is not available to others making changes to llvm node. We rely on the unit tests in this project.
Thanks again and if you have any questions just ask. I will try to clarify.
| @@ -364,6 +380,10 @@ declare namespace llvm { | |||
|
|
|||
| getTypeStoreSize(type: Type): number; | |||
|
|
|||
| getTypeAllocSize(type: Type): number; | |||
There was a problem hiding this comment.
I don't find this method in the LLVM documentation nor that it is implemented in this PR. should this (and the following method be declared on DataLayout instead)?
|
|
||
| auto* type = TypeWrapper::FromValue(info[0])->getType(); | ||
| auto* ptr = ConstantWrapper::FromValue(info[1])->getConstant(); | ||
| auto indexValues = v8::Array::Cast(*info[2]); |
There was a problem hiding this comment.
You should be able to use the toVector template from util/array for the conversion of the v8Array to a vector.
| auto dataLayout = DataLayoutWrapper::FromValue(info.Holder())->getDataLayout(); | ||
|
|
||
| auto size = dataLayout.getTypeAllocSize(type); | ||
| assert (size < UINT32_MAX && "V8 does not support uint64 but size overflows uint32"); // v8 does not support uint64_t :( |
There was a problem hiding this comment.
Wouldn't it be better to throw a nan exception here? The same applies for getTypeAllocSizeInBits
| @@ -13,54 +12,70 @@ NAN_METHOD(IntegerTypeWrapper::New) | |||
| } | |||
|
|
|||
| if (info.Length() < 1 || !info[0]->IsExternal()) { | |||
| return Nan::ThrowTypeError("Expected type pointer"); | |||
| return Nan::ThrowTypeError("Expected pointer type pointer"); | |||
There was a problem hiding this comment.
I don't understand the change here. Why is the additional pointer needed?
Signed-off-by: Vihan <[email protected]>
|
Hi @vihanb Thank you for fixing the review comments. However, the build is still failing. Can you take a look? I would prefer merging this soon to avoid that it gets any bigger. |
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
|
looks like still errors with IntegerTypes... will take a closer look at when I have more time in a few weeks |
|
So looks like I have |
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
Signed-off-by: Vihan <[email protected]>
So over the past year I've been working on adding various features to llvm-node for the language I've been working on. I first made a PR with #42 but I've put all my changes in one PR to be more organized and to avoid merge issues.
Anyway, so another thing is looks like both me and @ovr happened to add an
IntegerTypeclass but I've worked out the conflicts with those and all tests (for my language using llvm-node) pass.