Pull Request- Syntax Plugin Infrastructure.

[tpops]

Expansion of plugin infrastructure to improve flexibility for Syntax Handler Developers.

1. "Forget the function definition that triggered the plugin and insert the replacement tokens after it.
2. Syntax handler developers have access to the original name of the function using the Declarator object in the Get Replacement function call.
3. Modified the PluginSyntax example project to work with the new syntax handler plugin.
4. Modified the PluginSyntax test case so it tests for the new syntax handler plugin.

[amccaskey]

@tpops If I understand this correctly, this PR will take

clang::syntax(NAME) void foo() {
 ...
} 

and produce

void __foo() { // old, not used anymore
// empty
}
... REWRITTEN OUTPUT STREAM ...

Let me know if I'm wrong about this.

If thats the case, then this PR should work for our quantum computing use-cases / feature updates @hfinkel

[tpops]

Yes exactly

You can also get the original function declaration with the preprocessor and declarator objects.

auto DeclCharRange = Lexer::getAsCharRange (D.getSourceRange(), 
                    PP.getSourceManager(), PP.getLangOpts());
auto DeclText= Lexer::getSourceText (DeclCharRange,PP.getSourceManager(), 
                        PP.getLangOpts());
 OS << DeclText << "\n { \n } \n";

    `

[amccaskey]

👍

[hfinkel]

Thanks for working on this! It's clearly important that we support adding additional declarations/definitions after the processed function. I'm not convinced, however, that this is the best approach. It violates the principle of least surprise for the plugin to replace a function with a particular valid definition with another with some incompatible definition, and that's exactly what this allows. Plus, having the empty function around is not clearly desirable (and might generate warnings?).

Instead, I'd prefer that GetReplacement simply returned two streams, one for the replacement function body, and a second to be placed after the processed function. Is there some reason that approach is problematic or suboptimal?

[hfinkel]

Please follow LLVM's coding conventions here: Comments should be complete sentences, start with a capital letter, end with appropriate punctuation, etc.

[tpops]

done

[hfinkel]

Same here (plus there should be a space between the // and the text.

[tpops]

Okay, done.

[hfinkel]

What happens if the function does not return void? Don't we get a warning that the function doesn't return an appropriate value?

[tpops]

The idea of having two streams is a great idea. However, the following would be a problem:

1. The function been transformed would not have access to struct declarations made just after by the second stream.

// The function declaration is already parsed at this point.
[[clang::syntax(plugin)]] void doStuff(){
  // Streamed tokens replaced here would not have access to declarations 
  // such as function declarations and structures. 
  
  // You might want to have forward declaration for functions that would be 
  // generated by the second stream. However, forward declarations for structs / classes
  // defined by the second token stream would not work.
}

// Second stream tokens placed here

2. The current problem the compiler 'return' warnings could be solved by placing __builtin_unreachable(); in the forgotten function.

Least surprise is also a valid point.

[amccaskey]

I too am a fan of 2 streams. Is it possible to provide a third stream for function declarations that go before the transformed function, which we could then implement later in the second replacement stream after the closing function r_brace? Our use case has the same problem that @tpops just mentioned.

[tpops]

There actually exists

void AddToPredefines(llvm::raw_string_ostream &OS) override;

where you could add predefines, and predefined structs could be added here. However, this function is used by the plugin infrastructure before the GetReplacement is called. So there is no such thing as generating declarations after scanning the content of the transformed function.

My concerns is only for struct declarations that might want to be generated by scanning the tokens in the transformed function.

Function declarations can be solved easily by doing forward declaration

// The function declaration is already parsed at this point.
[[clang::syntax(plugin)]] void doStuff(){
  // Streamed tokens replaced here would not have access do declarations 
  // such as function declarations and structures. 
  
  // Forward declaration for doFoo
  void doFoo();
  doFoo();
}

// Second stream tokens placed here
void doFoo(){
  printf("doing foo");
}

[amccaskey]

Awesome, thanks! That's exactly what I would need. So yea, no need for anything separate just for function declarations.

[tpops]

@hfinkel I had a conversation with the rest of the AD group and here are the few key take away:

"I still do believe that function should be forgotten. The user input might not be sufficient in many ways. not creating a warning is a different issue we can address in various ways, including suppressing warnings for [[syntax(..)]] functions all together." - Johannes.

In complex implementations , code provided by the user might be used to generate structures and functions, this would be very tricky to work with for your two stream approach. In the case of our plugin implementation, we generate code from tensor operations provided by the user, the two stream approach would not be a favorable approach in this case.

[tpops]

Awesome, thanks! That's exactly what I would need. So yea, no need for anything separate just for function declarations.

You welcome. Glad I could be of help

[hfinkel]

I apologize for the delay in getting back to this. I recommend that we move forward with this, but we should do the following:

1. Add __builtin_unreachable to the original definition. This should take care of any warnings, and in addition, is a semantically-accurate description of what's going on.
2. Provide a utility function, wrapping what you put in the review above, to make it easy for the plugin to recreate the original definition. We should update the example plugin to use that utility function.

Any objections?

[tpops]

No Objections. Do you suggest, I create a new pull request for the new code changes or I go ahead and modify this request.

Would like to know which you would prefer @hfinkel

[hfinkel]

No Objections. Do you suggest, I create a new pull request for the new code changes or I go ahead and modify this request.

Would like to know which you would prefer @hfinkel

I have no preference. Thanks!

[tpops]

Sounds good. Just to reiterate the updated commits on current pull request:

1. Forgets the function definition that triggered the plugin and insert the replacement tokens after it.
2. Adds __builtin_unreachable to the original definition.
3. Provides a utility function, getDeclText(Preprocessor &PP,Declarator &D); .
4. Updates the example to use getDeclText.