InstSelector restructuring and addition of FuncName instruction selector.

[ShadenSmith]

Controller now has a FIInstSelectorManager, which maintains a list of all InstSelectors that the user chooses. Each selector creates its own set of allowed instructions, and then the manager computes the set intersection of all of them and returns.

This is a cleaner solution than what was previously discussed on the mailing list. Making a special InstSelector to contain others and AND them together required changing some of the class layout for FIInstSelector because a sibling class would need access to private data. This method is more flexible for future development.

A major advantage of this design is the simplicity of adding selectors. The input YAML file now expects a a list of selectors to use. A new test program has been added, which uses both funcname and insttype selectors and demonstrates the power of using both at once.

[jshwei]

I had a brief look at the code, and I like the design here. We will merge it after having a deep look and some tests.

Two things I think we can improve on:

1. instead of keep adding 'special cases' to inst selector, we might want to make no difference between all of the existing special cases (inst type, func name, source code) and custom selector. Basically, we remove register all inst selector by their name, and take selector name and selector options for each of them.
2. how do we enable OR operations of selectors, instead of only AND?

It would be great if you can work on it later. If not, at least we keep a record of this, so that people who are interested can refactor the code later.

[ShadenSmith]

Hi Jiensheng,

Thanks again for the valuable feedback. Please let me know if you find any issues.

instead of keep adding 'special cases' to inst selector, we might want to make no difference between all of the existing special cases

I agree, this is a very good idea. If my understanding is correct, the custom selector implementation does not currently support receiving input from the command line. That is primarily the reason that I did not make the function name selector a custom one.

If we instead create a system in which adding arbitrary selectors is simple (including their input!) then I think this is a good solution. One thought is to remove the input validation logic from instrument.py and instead allow LLVM's command line parser do the work. Any of LLVM's stdout could then be passed to the user. A simple way of avoiding argument name clashes is to give each selector a handle (say, "inst" or "func") and prefix all of their arguments with that handle. This similar to what we have now with instruction type and function name.

how do we enable OR operations of selectors, instead of only AND?

I think it should be as simple as using set_union instead of set_intersection. I haven't actually tried that, however.

Thanks, Shaden

[flyree]

Hi Shaden,

I think it is a very interesting extension. I have one concern here though: can you clarify how we will use customselector after applying your change? It seems to me that the custom selector would be a little tricky to use within your interface.

Again, thank you for your contribution.

Bo

[ShadenSmith]

Hi Bo,

My understanding is that the current state of my code should handle custom selectors just as they were before, only now their results will be ANDed with the other selectors in use. You would use one by adding it to the list of selectors in the YAML file. If that is incorrect, please let me know and I can patch my code.

The possible design that myself and Jiesheng are referring to proposes to remove custom selectors entirely. Right now we don't have a way of giving input to them without a configuration file separate from the YAML file, so as a user I would prefer to just add another full-featured selector like FuncName. What we're proposing is to find a way to simplify the addition of new selectors and merge with custom selectors. Jiesheng, please let me know if I misunderstood your post. I'll spend some time today to come up with a more formal proposal for this design.

• Shaden

[ShadenSmith]

Hi Bo,

I understand your concern now. The previous code was not correctly telling the llvm_pass to use a custom fault selector. Things should be working now. Here is a sample use of multiple instruction selectors. including a custom one:

 instSelMethod:
      - custominstselector: onlymain
      - funcname:
          include: 
            - all
          exclude:
            - sum
      - insttype:
          include:
            - all
          exclude:
            - ret

NOTE: One consequence of the current architecture is that only one custom instruction selector can be used at once.

Best, Shaden

[ShadenSmith]

I found a bug with this code in which C++ functions could not be targeted due to name mangling. Names are now demangled during instruction selection (see Utils.cpp).

When templated and overloaded functions are targeted, ANY function with the same id will be selected. If we want to be able to target overloaded or templated functions only with a specific type, that's something that will need to be changed. The current design seems sane to me, though.

[jshwei]

This is a very good change! we are going to test it and merge them. Thanks.

[jshwei]

Thanks Shaden, the change is merged (finally :) ).

Coming back to the your point: "I agree, this is a very good idea. If my understanding is correct, the custom selector implementation does not currently support receiving input from the command line. That is primarily the reason that I did not make the function name selector a custom one.". Actually we do pass command line arguments to custom inst selector (have a look at LLFIIndexFIInstSelector.cpp). Although this is not a super clean solution, but I still think that's a better direction to go: we should move the arguments to corresponding selector's file, and make all selectors have the same privilege.