I think we can move this check into TransformationAddParameter::IsParameterTypeSupported.

We could also add support for Workgroup storage class. It's the same as Private storage class but the initializer is 0. See TransformationAddGlobalVariable docs for more details.

This part needs some refactoring. GetIRContext()->module()->ForEachInst iterates over all instructions in the module. This includes all global OpVariables and all local ones. The part when we iterate over all local variables needs some thought. See #3627 (comment) for more elaborate explanation.

I'm not sure whether this is a good or a bad practice to mark a variable initializer as irrelevant. @paulthomson, what do you think? In case we decide to make it irrelevant, it would be good to add a comment similar to

We mark the constant as irrelevant so that we can replace it with a more interesting value later.

I think it works.

This will create a local OpVariable instruction in the function. This is somewhat tricky.

The initial idea is that the created variable will be used as an operand to OpFunctionCall instructions. These instructions can belong to different functions - we'll call them callers - (since we can call any function from any other as long as there are no recursion - SPIR-V doesn't support that). Thus, this piece of code should create a local variable in every caller of function.result_id(). This is somewhat tricky because some callers might already have a suitable local variable.

I propose to refactor this fuzzer pass as follows:

1. Check the storage class of the variable. If it's Private or Workgroup, either find a global variable (IRContext must have a method that returns only global instructions) or create a new one (as you do above). Otherwise...
2. Iterate over all callers of function.result_id() (see fuzzerutil::GetCallers) and for each one either find a suitable local variable or create one.

I would also suggest to change the protobuf file to pass a repeated field of UInt32Pair (see fuzzerutil::Repeated*ToMap and fuzzerutil::MapToRepeated*). This field would represent a map from the result id of the caller to the result id of the local variable in that caller.

You would also need to adjust IsApplicable method in the transformation class to check that each caller has a local variable (if the added parameter is indeed a pointer with Function storage class). Hope this makes sense.

I was thinking about the adding of a map to a protobuf file and for me it seems unneccessary. I came up with two reasons:

1. It would mean refactoring the code in cases where AddParameter doesn't add a pointer with Function storage class.

2. In the case where AddParameter is about to add the pointer with Function storage class, the information whether the transformation is applicable or not is already accessible, since we pass function_id. And as you said, we could iterate over all callers and check if there is a local variable in each calling function. Even if we pass the map in the protobuf we still have to recreate it in the IsApplicable method by iterating over all callers to check whether the resulting two maps are equal. For example, even if we have checked that each key in the passed protobuf map refers to a valid local variable, we might have missed some callers. The actual check is in iterating over the callers in IsApplicable method.

Yes, you are right. It is indeed might be better to simply check in the transformation that the required parameter is available and in the fuzzer pass to ensure that the parameter is available. In this case, I would've created two functions in the fuzzerutil: MaybeGetLocalVariable and MaybeGetGlobalVariable (or some equivalent of these) that return result id of the local/global variable or 0 if no such variable exists.

You could then pass the type of the newly created parameter to the transformation and check that the variable (or the constant) of the required type exists.

There is one caveat, though. Currently, we do the opposite: we pass the value of the parameter and infer parameter's type out of it. You will need to change that.

The function-creation part is up to you, though.

initializer_id is used to initialize the OpVariable instruction. There is also an initializer_id argument in the transformation's constructor that is used to initialize an operand in OpFunctionCall instruction. In our case, the latter is the result id of the OpVariable.

Sorry, it's a bit confusing but the bottom-line is that you should use the result id of the variable here instead of the initializer_id.

It would be better to make this just argument_type_id that accepts a type id of the argument. In this case, you would need to validate that the type is supported by the transformation and:

1. If type is a scalar or a composite - check that there is an irrelevant constant in the module (fuzzerutil::MaybeGetZeroConstant)
2. If it is a pointer - check that a suitable variable is present in the module (we haven't decided yet on what facts the variable should posses)

Or maybe parameter_type_id to be consistent with the naming.

It would be good to add two boolean parameters to these functions:

1. pointee_value_is_irrelevant
2. id_is_irrelevant

and make sure the the returned id is marked (or not marked depending on the value of parameters) with those two facts accordingly.

Also, mention these parameters in the doc.

Missing a new line.

Mention that type_id is required to be a result id of OpTypePointer instruction with Function storage class.

Mention that type_id is a result id of an OpTypePointer instruction with Private or Workgroup storage class.

You can move the comment into the scope like so:

} else {
  // comment

Consider using opt::analysis::Type::AsPointer as I've described in the fuzzer pass.

You can get rid of is_valid and return false directly.

Nice!

It would be good to make an assert here saying that storage class is not supported.