Skip to content

ARROW-7819: [C++][Gandiva] Add DumpIR to Filter/Projector object - #6417

Closed
fsaintjacques wants to merge 2 commits into
apache:masterfrom
fsaintjacques:ARROW-7819-gandiva-dump-ir-tool
Closed

ARROW-7819: [C++][Gandiva] Add DumpIR to Filter/Projector object#6417
fsaintjacques wants to merge 2 commits into
apache:masterfrom
fsaintjacques:ARROW-7819-gandiva-dump-ir-tool

Conversation

@fsaintjacques

@fsaintjacquesfsaintjacques commented Feb 13, 2020

Copy link
Copy Markdown
Contributor

The following patch exposes the generated IR as a method of the objects
for further inspection. This is a breaking change for the internal
method FinalizeModule which doesn't take the dump_ir and optimize
flags, it receives optimize from Configuration now.

  • Refactored Engine, notably removed dead code, organized init in a single
    function and simplified LLVMGenerator.
  • Dumping IR should not write to stdout, but instead return it as a
    string in the DumpIR method.
  • Refactored Types, fixing some bad methods type.
  • Added the optimize field to Configuration class.
  • Simplified some unit tests.

But more importantly, we can now inspect dynamically:

>>>filter=gandiva.make_filter(table.schema, condition) >>>print(filter.ir)
; ModuleID='codegen'source_filename="codegen"targetdatalayout="e-m:e-i64:64-f80:128-n8:16:32:64-S128"targettriple="x86_64-unknown-linux-gnu"@llvm.global_ctors=appendingglobal [0x { i32, void ()*, i8* }] zeroinitializer@_ZN5arrow7BitUtilL8kBitmaskE=internalunnamed_addrconstant [8xi8] c"\01\02\04\08\10 @\80", align1; FunctionAttrs: norecursenounwinddefinei32 @expr_0_0(i64*nocapturereadonly%args, i64*nocapturereadonly%arg_addr_offsets, i64*nocapturereadnone%local_bitmaps, i16*nocapturereadnone%selection_vector, i64%context_ptr, i64%nrecords) local_unnamed_addr#0 {entry: %0=bitcasti64*%argstoi8**%cond_mem56=loadi8*, i8**%0, align8%1=getelementptri64, i64*%arg_addr_offsets, i643%2=loadi64, i64*%1, align8%a_mem_addr=getelementptri64, i64*%args, i643%3=bitcasti64*%a_mem_addrtodouble**%a_mem7=loaddouble*, double**%3, align8%scevgep=getelementptrdouble, double*%a_mem7, i64%2brlabel%looploop: ; preds=%loop, %entry%loop_var=phii64 [ 0, %entry ], [ %"loop_var+1", %loop ] %scevgep8=getelementptrdouble, double*%scevgep, i64%loop_var%a=loaddouble, double*%scevgep8, align8%4=fcmpoltdouble%a, 1.000000e+03%5=sexti1%4toi8

@github-actions

Copy link
Copy Markdown

@fsaintjacques
fsaintjacquesforce-pushed the ARROW-7819-gandiva-dump-ir-tool branch from 6ab97b0 to 2dc7955CompareFebruary 13, 2020 17:13

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM in general, a few comments.

Comment threadcpp/src/gandiva/arrow.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't we use type_fwd.h instead?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I tried removing the others and minimizing but it unfolded in too many changes.

Comment threadcpp/src/gandiva/engine.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm... can we keep the includes as minimal as possible? Arrow is already slow enough to build.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

clangd has this option to automatically add header when auto-completing. I disabled it.

Comment threadcpp/src/gandiva/engine_llvm_test.cc Outdated
Comment threadpython/pyarrow/gandiva.pyx Outdated
Comment threadpython/pyarrow/tests/test_gandiva.py Outdated
Comment threadpython/pyarrow/tests/test_gandiva.py Outdated
The following patch exposes the generated IR as a method of the objects
for further inspection. This is a breaking change for the internal
method `FinalizeModule` which doesn't take the dump_ir and optimize
flags, it receives `debug` from Configuration now.
- Refactored Engine, notably removed dead code, organized init in a single
function and simplified LLVMGenerator.
- Dumping IR should not write to stdout, but instead return it as a
string in the `DumpIR` method.
- Refactored Types, fixing some bad methods type.
- Added the optimize field to `Configuration` class.
- Simplified some unit tests.
@fsaintjacques
fsaintjacquesforce-pushed the ARROW-7819-gandiva-dump-ir-tool branch from 2dc7955 to 0bcebc8CompareFebruary 13, 2020 21:51

@projjalprojjal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

llvm::Function* fn = module->getFunction(iter.pc_name());
EXPECT_NE(fn, nullptr) << "function " << iter.pc_name()
<< " missing in precompiled module\n";
EXPECT_NE(module->getFunction(iter.pc_name()), nullptr);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

having the name in stderr is helpful for debugging.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I recommend to set this in your .gdbinit

set environment GTEST_BREAK_ON_FAILURE=1

Then you run the failing test under gdb, it'll break at the first failing test.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

He might mean that the problem may be more evident from looking at test log files :) I think it's OK for now, if it becomes an issue we can improve

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes, wes - I meant easier to get error from log files or travis output !. but, we'll only hit this when adding new functions - so, it's obvious anyway.

Sign up for freeto 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.

5 participants

@fsaintjacques@wesm@pitrou@pravindra@projjal