cbalint13 commented on PR #15761:
URL: https://github.com/apache/tvm/pull/15761#issuecomment-1727977364
@kparzysz-quic ,
Thanks again taking time !
> The idea behind `LLVMInstance` is that all interactions with the LLVM
libraries only happen when an object of that class has been created. You can
think of it as if "LLVM instance" was turning on LLVM, and turning it back off
when the object is destroyed. There was a discussion about this a while back,
motivated mostly by keeping track of the global state of LLVM. Long story
short, LLVM code has a global state, and modifying the global state in one part
of TVM may affect how LLVM works in other parts (e.g. setting up codegen for
CPU may affect codegen for GPU). LLVM instance was created to encapsulate any
potential global state changes into the lifetime period of the LLVM instance
object.
* I reflected on this way (as being the best place) when wrote this PR, but
now explaining it is more clear.
> The functions you wrote can be moved to llvm_instance.cc. They can create
a temporary LLVMInstance object, get LLVMTargetInfo, get the `llvm::Target`
object (or `llvm::TargetMachine`) from it, and do their work.
* If it is fine to create-destroy a whole ```LLVMInstance()``` and appeal
methods from it, lets go.
* Worried about fact that even for single FFI query, maybe repeated many
times in the hot-path iterations, to create/destroy/appeal whole LLVMInstance
(plus sub-LLVM related) might be bad.
* I was worrying that already have to go through (LLVM point of view) a
whole 2*inits() ->register-target->target-machine->subtarget->query (LLVM way)
just for the sake of "one simple" info. But that's it, if we want this.
> Btw, the functionality you're adding is great, what I'm asking for is to
integrate it with the existing code.
Thank you much for the feedback !
* One thing I really keen was the ability to tell info on any LLVM version.
I really insisted on making it backward compat, even for earliest LLVM. It
makes sure about LLVM limits, e.g. some recentish "super-dooper" CPU cant use
it's X or Y feature because your LLVM is older (and user doesn't know), so
don't even try schedule whatever nice tensorization steps for the user. Perhaps
even warn the user that his LLVM is old, but upgrading he will benefit.
> Edit: the query functions you have written don't need to be members of
`LLVMTargetInfo` if that , but they should create an `LLVMInstance` object and
only call LLVM functions during the lifetime of that object.
* Maybe, in future we could make this object a persistent one, even cache
some things inside, but I would put this idea aside for future. Maybe at a
time, it will be decided on some way to migrate more arches (all?) towards this
way of query things, perhaps even at earliest tvm.target.Target("llvm -xxx")
creation step.
> The idea behind `LLVMInstance` is that all interactions with the LLVM
libraries only happen when an object of that class has been created. You can
think of it as if "LLVM instance" was turning on LLVM, and turning it back off
when the object is destroyed. There was a discussion about this a while back,
motivated mostly by keeping track of the global state of LLVM. Long story
short, LLVM code has a global state, and modifying the global state in one part
of TVM may affect how LLVM works in other parts (e.g. setting up codegen for
CPU may affect codegen for GPU). LLVM instance was created to encapsulate any
potential global state changes into the lifetime period of the LLVM instance
object.
> The functions you wrote can be moved to llvm_instance.cc. They can create
a temporary LLVMInstance object, get LLVMTargetInfo, get the `llvm::Target`
object (or `llvm::TargetMachine`) from it, and do their work.
>
> Btw, the functionality you're adding is great, what I'm asking for is to
integrate it with the existing code.
>
> Edit: the query functions you have written don't need to be members of
`LLVMTargetInfo` if that , but they should create an `LLVMInstance` object and
only call LLVM functions during the lifetime of that object.
See now, again thanks much for the time !
Let me try (1-2 day), as you suggested, then if don't mind will re-ask to
help again with a review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@tvm.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
