Page MenuHomePhabricator

[expression evaluator] Allow runtimes to execute custom LLVM ModulePasses over the generated IR at various stages after expression compilation.
AbandonedPublic

Authored by ldrumm on Feb 9 2016, 7:04 AM.

Details

Reviewers
spyffe
jingham
Summary

During expression evaluation, the ClangExpressionParser preforms a number of hard-coded fixups on the expression's IR before the module is assembled and dispatched to be run in a ThreadPlan.

This patch allows the runtimes to register LLVM passes to be run over the generated IR, that they may perform language or architechture-specfic fixups or analyses over the generated expression.

This makes expression evaluation for plugins more flexible and allows language-specific fixes to reside in their own module, rather than littering the expression evaluator itself with language-specific fixes.

Diff Detail

Event Timeline

ldrumm updated this revision to Diff 47315.Feb 9 2016, 7:04 AM
ldrumm retitled this revision from to [expression evaluator] Allow runtimes to execute custom LLVM ModulePasses over the generated IR at various stages after expression compilation..
ldrumm updated this object.
ldrumm added reviewers: jingham, spyffe.
ldrumm added a subscriber: lldb-commits.
jingham edited edge metadata.Feb 9 2016, 10:28 AM

This seems fine as a generic instrumentation point. Obviously, the onus in on the passes, since they could totally ruin the expression evaluation if they don't do their job right... But I'm not sure there's any good way to make this not a sharp tool. But you should wait on Sean to weigh in as well...

One question, what is the force of the "legacy" in "llvm::legacy::PassManager? Is there some more "modern" way to do this, is this going to go away at some point?

This seems fine as a generic instrumentation point. Obviously, the onus in on the passes, since they could totally ruin the expression evaluation if they don't do their job right... But I'm not sure there's any good way to make this not a sharp tool. But you should wait on Sean to weigh in as well...

I appreciate this, and I think that if a runtime provides passes, the expectation should be that the runtime knows what it's doing because this really is a powertool - as you say. I think it's a generally useful mechanism for better modularity/encapsulation of language-specific behaviours that are required during expression evaluation. For example, there are presently a large number of language-specific fixups in the clang-based expression evaluator, that could be factored out into individual EarlyPasses/LatePasses provided by their runtime, leaving IRForTarget and friends a lot cleaner and more maintainable.

One question, what is the force of the "legacy" in "llvm::legacy::PassManager? Is there some more "modern" way to do this, is this going to go away at some point?

The legacy pass manager is fully functional and tested. Using the new version does not work unfortunately in this instance. We can certainly move to the new version as it matures and becomes ready for general use, but I tried this implementation with both llvm::PassManager and the the llvm::legacy::PassManager, and unfortunately the new PassManager did not work here.

ldrumm added a subscriber: ldrumm.EditedFeb 15 2016, 12:23 PM

Hi Jim

<snip>

Did you file bugs with llvm on these issues?  It would be good to file a "move to the new pass manager when ready" bug on lldb, but it should be blocked on the relevant llvm bugs.

Not yet. I believe it's best to wait this until this revision is accepted before reporting bugs on its implementation. Though I'm happy to do so, I'll prefer to wait for yours and Sean's final comments.

Let me know your thoughts.

Best

Luke

Hi @spyffe @jingham

If you have the time, I'd appreciate your comments.

Thanks

Luke

Hi Sean

Did you get a chance to look over this? It'd be great to have your input.

Best

Luke

ldrumm updated this revision to Diff 57199.May 13 2016, 8:48 AM
ldrumm edited edge metadata.

rebased on current HEAD

gentle ping, if @spyffe or @jingham is available to take another pass at this.

I'd like to give this another *bump*, and commit this soon assuming positive review.

@spyffe