Bring ltimes benchmark up to date #1738
Conversation
| @@ -29,7 +29,7 @@ namespace expt | |||
| { | |||
|
|
|||
|
|
|||
| template<typename IDX, typename TENSOR_TYPE, camp::idx_t DIM, IDX INDEX_VALUE, strip_index_type_t<IDX> LENGTH_VALUE> | |||
There was a problem hiding this comment.
Support for strongly typed indices with TensorIndex was missing, and that is fixed here. We don't have this in our CI testing, so I'll create an issue to add tests for that.
scripts/lc-builds/toss4_amdclang.sh
Outdated
| @@ -73,6 +73,7 @@ cmake \ | |||
| -DENABLE_HIP=ON \ | |||
| -DENABLE_OPENMP=ON \ | |||
| -DENABLE_CUDA=OFF \ | |||
| -DENABLE_BENCHMARKS=ON \ | |||
There was a problem hiding this comment.
Should we make this sort of thing an argument you can pass to the script like we do for compiler version, compute architecture, etc?
scripts/lc-builds/blueos_nvcc_gcc.sh
Outdated
| shift 3 | ||
| BENCHMARKS=Off | ||
| BUILD_SUFFIX=lc_blueos-nvcc${COMP_NVCC_VER}-${COMP_ARCH}-gcc${COMP_GCC_VER} | ||
| fi |
There was a problem hiding this comment.
@rhornung67 and others, what do we think of doing it this way? If we turn on benchmarks we append a note on the folder name?
There was a problem hiding this comment.
That's fine, but I don't think it's necessary. We only have a few benchmark problems, so building them is not a big deal. We could just enable benchmarks in all our scripts and folks can turn them off easily enough if they want. We ill enable them in a subset of CI builds.
There was a problem hiding this comment.
That works for me, just to double check. Turn them on for all scripts, and have the command option to turn them off? I'll also leave out the appending note on the folder name. Does that work for all?
There was a problem hiding this comment.
I don't think a command line option is needed. We have to edit the build script files to enable/disable features, back-ends, etc. Having a special command line arg for this one case seems confusing and less convenient for me.
There was a problem hiding this comment.
Okay, I can revert and just turn them on across the board. Is this right?
|
I think this is good to go now. |
|
@artv3 Did we want to turn on building |
|
@artv3 if you don't need/want the branch from this PR, please delete. Thank you. |
Summary
This PR bring the ltimes kernel up to date. It is currently a WIP.