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

irutil: add instruction and terminator operand use-tracking and value update API #1

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

mewmew
Copy link
Member

@mewmew mewmew commented Dec 21, 2019

Rather than adding the use-tracking to llir/llvm/ir, we currently add
this to irutil so we may get more experience with the API and see if its
the right fit for instruction and terminator operand use-tracking and
value update.

Note: this is the stronger version of the API, as outlined by @pwaller
in llir/llvm#42 (comment)

Notably, operands are tracked by pointer (*value.Value) which also enables
updating values using the same API.

Fixes llir/llvm#42.

Updates llir/llvm#19 (note, we don't do deep use tracking in this PR, so llir/llvm#19 is only partially fixed).

cc: @pwaller and @dannypsnl

… update API

Rather than adding the use-tracking to llir/llvm/ir, we currently add
this to irutil so we may get more experience with the API and see if its
the right fit for instruction and terminator operand use-tracking and
value update.

Note: this is the stronger version of the API, as outlined by @pwaller
in llir/llvm#42 (comment)

Notably, operands are tracked by pointer (*value.Value) which also enables
updating values using the same API.

Fixes llir/llvm#42.
go.mod Outdated Show resolved Hide resolved
…tUses

Note: we should try to arrive at a more general way of handling custom
user-defined instructions.
use.go Outdated Show resolved Hide resolved
…ue operands

Without this change, repeated invokations of InstUses (as used in FuncUses)
would result in incorrect results.

	for _, inst := range block.Insts {
		backing = InstUses(backing, inst)
	}
Since we already track a pointer to the used value in the Use structure,
we can pass Use by value. This should help reduce memory allocations.
@pwaller
Copy link
Member

pwaller commented Dec 23, 2019

Further thoughts.

I think that InstUses as you have it here is really interface { OperandsAsUses([]Use) []Use }, and should be methods on the individual types.

  1. Including the word Operands somewhere in the name eliminates the confusion with Uses. In the LLVM API "Inst.Uses()" means to obtain where the instruction is used, not to make a list of uses out of the operands of the instruction as I understand we're doing here (please correct me if there is an edge case I've missed).
  2. Having a method on the type makes it clearer that it is a property of instances of the type, as opposed to some algorithm which will compute the uses of an instruction.
  3. This allows dynamic dispatch, rather than at runtime having to iterate through all the possible types, you can directly call just the relevant code (as I understand it, but I've not looked and done the measurements, so I could be mistaken - caveat emptor).
  4. This solves the "custom" case. If anyone wants to implement OperandsAsUses, they can.

I realise this means that type Use would have to go in the IR package, and you're reluctant to put it there. For what it's worth, Value and Use live in the same library in llvm, and this makes some sense to me. The definition of Use is so small and so often useful, I don't currently see why you'd want to put it in a different package.

I envisage a use.go in the ir package which contains all of the use methods for the different types.

A note on naming: backing came to me very rapidly. I think I might prefer in and out in this usecase:

func foo(in []bar) (out []bar) {
  out = in
  ...
  out = append(out, extra...)
}

... but please adjust to your (good) taste! :)

The other thing in the back of my mind is - how are we going to maintain the sets of UsesOf Inst => []Use? Especially once you start manipulating the IR. In LLVM I understand that each value maintains a UseList within it.

Note: this implementation of dce is a toy example. It does not take deep
value uses into account, and as such, cannot handle use tracking when
local variables are used within constant expressions.

This issue would be resolved when irutil.FuncUses is extended to handle
deep value use tracking.
@mewmew
Copy link
Member Author

mewmew commented Dec 24, 2019

I've added an example use case of the API to get a better feel for the use-tracking API.

For use-tracking, see the dead code elimination example at: https://github.com/llir/irutil/blob/503ea997aa34863b1a261bd5c0a93a46245cb371/example_test.go

And for used value modification, see the NOP trunc example at: https://gist.github.com/mewmew/05d625cc101d81e4f7b79469f41105fe

@mewmew
Copy link
Member Author

mewmew commented Dec 24, 2019

I think that InstUses as you have it here is really interface { OperandsAsUses([]Use) []Use }, and should be methods on the individual types.

Indeed, {Inst,Term}Uses is really interface { OperandsAsUses([]Use) []Use }.

In the LLVM API "Inst.Uses()" means to obtain where the instruction is used, not to make a list of uses out of the operands of the instruction as I understand we're doing here

Yeah, using InstUses creates a confustion as it's too close to Inst.Uses() of the official LLVM API. Your proposed OperandsAsUses makes sense and I like it. And as you predicted, I don't want to put it in llir/llvm/ir just yet.

Having a method on the type makes it clearer that it is a property of instances of the type, as opposed to some algorithm which will compute the uses of an instruction.

Agreed. Using methods for OperandsAsUses is most likely what we would do for the v0.4 release proper. Until then, I think we can live with func irutil.InstOperandsAsUses(in []Use, inst ir.Instruction) (and analogous for TermOperandsAsUses). It is a bit ugly, but lets us experiment and get a feel for the API. I know this doesn't solve the "custom" instructions issue, but I imagine we can introduce those functions in irutil, and still add the interface interface { OperandsAsUses([]Use) []Use }, which we could then use to check "custom" instructions.

This allows dynamic dispatch, rather than at runtime having to iterate through all the possible types, you can directly call just the relevant code (as I understand it, but I've not looked and done the measurements, so I could be mistaken - caveat emptor).

Yeah, methods is most likely the way to go, for v0.4 that is :)

This solves the "custom" case. If anyone wants to implement OperandsAsUses, they can.

Thanks for sharing your thoughts on this Peter! I think it definitely helped us move closer to a good use-tracking API, and solves the already identified issue of "custom" instructions ("custom" anything use-tracking related really, e.g. perhaps a "custom" constant expression).

I envisage a use.go in the ir package which contains all of the use methods for the different types.

Agreed.

The other thing in the back of my mind is - how are we going to maintain the sets of UsesOf Inst => []Use? Especially once you start manipulating the IR. In LLVM I understand that each value maintains a UseList within it.

I have no idea yet :) That's essentially the main reason I want to keep the use-tracking API in irutil for now, as it's experimental, and only by iterative design we will arrive at something solid enough to be "promoted" into llir/llvm proper. That would coincide with the v0.4 release of llir/llvm.

mewmew added a commit to mewpull/leaven that referenced this pull request Dec 30, 2019
Note, the v0.3.0 release had a few changes to the API. The most
intrusive one may be to use value.Value in all places where
operands of instructions are used. This has both pros and cons,
where the cons include less exact types used for instruction and
terminator operands. The benefit is that this change enable a
unified and quite powerful use-tracking API, as further outlined
in llir/llvm#50, llir/llvm#122 and llir/irutil#1.
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.

Add an Operands() function for inspecting operands of instructions
2 participants