Skip to content

Property refactor v2

Arthur Marius Hennequin requested to merge ahennequ_property_refactor into master

Continuation of !1294 (closed)

This MR is a proposal for a syntax change for properties in Allen, in accordance to the Gaudi syntax. It reduces the duplication in the previous property syntax for algorithms.

Proposed changes

  • The developed syntax with this MR is:

    Allen::Property<float> m_pre_scaler {this, "pre_scaler", 1.f, "Pre-scaling factor"};

    Properties could be accessed before with m_pre_scaler.get_cached_value(). As a more straightforward approach, this MR allows to access the properties just using m_pre_scaler (which provides an override of the operator T() cast operator), or m_pre_scaler.value() in case that's needed.

  • Properties are no longer passed by default to device calls with the parameters. Therefore, they have to be explicitly passed to a kernel invocation. This has the benefit that only those properties that are required can be passed. It could be achieved before by not specifying an instance name in the PROPERTY definition, but that was hard to notice. Eg. of invocation of SearchByTriplet kernel with three properties:

    global_function(velo_search_by_triplet)(size<dev_event_list_t>(arguments), m_block_dim_x.get(), context)(
      arguments, constants.dev_velo_geometry, m_tolerance, m_max_scatter, m_max_skipped_modules);
  • Line definitions require properties in the select and 'fill_tuples' functions. The invocation to the lines are done behind the scenes, so one cannot unfortunately explicitly pass the properties. The way to do this would be to define a struct DeviceProperties with the desired properties and a constructor. Eg. of definition of SingleHighETLine.cuh:

    struct single_high_et_line_t : public SelectionAlgorithm, Parameters, Line<single_high_et_line_t, Parameters> {
      ...
      // Properties available on the device
      struct DeviceProperties {
        float minET;
        Properties(const single_high_et_line_t& algo) : minET(algo.m_minET) {}
      };
    
      // Selection function
      __device__ static bool select(const Parameters& parameters, const DeviceProperties& properties, std::tuple<const float> input);
    
      __device__ static void fill_tuples(
        const Parameters& parameters,
        const DeviceProperties& properties,
        std::tuple<const Allen::Views::Physics::BasicParticle> input,
        unsigned index,
        bool sel);
    
    
    private:
      // Line-specific properties
      Allen::Property<float> m_minET {this, "minET", 15000.f, "min Et of brem cluster"};
    };

    Then, the select function has independent access to Parameters and DeviceProperties in two different objects.

Changelist

  • All algorithm's PROPERTY and Property is now unified following the new syntax.
  • All __global__ functions that were using properties now explicitly have the properties as parameters of the function, which are passed in the function call.
  • All lines select and fill_tuples functions required passing the properties and using them. To this end, a DeviceProperties struct (see above) has been created with an according constructor based on the properties the line requires.
  • DeviceAccumulators structs that were you used in lines with monitoring, are now merged with DeviceProperties such as only one struct needs to be created.
  • Classes Property and BaseProperty have been adapted following @graven 's comments (see original MR).
  • Backend code to support properties inside the Parameters struct and tuple has been removed as it is now unnecessary.
  • The type DeviceDimensions has been removed. Instead, the CUDA-native dim3 type (which already had a CPU-backend version) is used.
  • The parsing of properties has been reworked to adapt to the new syntax. In particular it is now using the default_properties executable to get the value, name, type and description and properties, instead of relying on clang parser. This allows properties to be correctly inherited from base classes.
  • Common line properties have been moved to base class SelectionAlgorithm
  • Documentation has been updated accordingly.

Closes !1294 (closed)

Edited by Arthur Marius Hennequin

Merge request reports

Loading