search

ROCm / Tensile

Stretching GPU performance for GEMMs and tensor contractions.
MIT License
225 stars 151 forks source link

Add logic to keep build_tmp when running Tensile #2018

Closed ellosel closed 2 months ago

ellosel commented 3 months ago

Summary:

Changes were added to TensileCreateLibrary to remove (or not) the build_tmp directory. However, the same changes were overlooked in Tensile. This PR adds the logic necessary to remove (default) or keep build_tmp via the --keep-build-tmp option.

Outcomes:

Temporary source files and build artifacts are available for inspection after running Tensile command.

Notable changes:

Adds --keep-build-tmp option to Tensile application.

Testing and Environment:

Local testing ran using a config file provided by @nakajee. After reviewing tensile ouput directory, temporary build files are removed (or not) depending on whether option is set.

ellosel commented 3 months ago

@nakajee should the yaml or command line option take precedence?

nakajee commented 2 months ago

@nakajee should the yaml or command line option take precedence?

Not sure. Any other parameters for reference?

ellosel commented 2 months ago

@nakajee should the yaml or command line option take precedence?

Not sure. Any other parameters for reference?

There aren't. For this to work, we have to give precedence to the yaml file. If it isn't specified in the file then we can fall back to the command line option (which will default to removing temporaries).

nakajee commented 2 months ago

@nakajee should the yaml or command line option take precedence?

Not sure. Any other parameters for reference?

There aren't. For this to work, we have to give precedence to the yaml file. If it isn't specified in the file then we can fall back to the command line option (which will default to removing temporaries).

OK. The only thing is it is a little difficult to recognize this global parameter without having it in init part. Maybe better to put some comment for this parameter? Or, any better idea?

ellosel commented 2 months ago

@nakajee should the yaml or command line option take precedence?

Not sure. Any other parameters for reference?

There aren't. For this to work, we have to give precedence to the yaml file. If it isn't specified in the file then we can fall back to the command line option (which will default to removing temporaries).

OK. The only thing is it is a little difficult to recognize this global parameter without having it in init part. Maybe better to put some comment for this parameter? Or, any better idea?

Another option is to drop the command line option and only support setting it via the config file. Then we could move it all into common. That said, there are a few others like this in the Tensile main.

nakajee commented 2 months ago

The change looks good. Please update year in each header comment (as I said in another PR).