Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use phase1PixelTopology::numberOfLayers where appropriate #33244

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Use phase1PixelTopology::numberOfLayers where appropriate #33244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad97afe
Use phase1PixelTopology::numberOfLayers where appropriate
czangela Mar 22, 2021
4d1f89a
clang format
czangela Mar 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Geometry/TrackerGeometryBuilder/test/phase1PixelTopology_t.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,9 @@ int main() {
}

for (auto i = 0U; i < phase1PixelTopology::numberOfModules; ++i) {
int layer = phase1PixelTopology::layer[i / phase1PixelTopology::maxModuleStride];
auto layer = static_cast<const uint32_t>(phase1PixelTopology::layer[i / phase1PixelTopology::maxModuleStride]);
//std::cout << "module " << i << ": " << "layer " << layer << ", \"" << phase1PixelTopology::layerName[layer] << "\", [" << phase1PixelTopology::layerStart[layer] << ", " << phase1PixelTopology::layerStart[layer+1] << ")" << std::endl;
assert(layer < 10);
assert(layer < phase1PixelTopology::numberOfLayers);
assert(i >= phase1PixelTopology::layerStart[layer]);
assert(i < phase1PixelTopology::layerStart[layer + 1]);
}
Expand Down
11 changes: 8 additions & 3 deletions RecoLocalTracker/SiPixelRecHits/plugins/SiPixelRecHitSoAFromLegacy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,11 +236,16 @@ void SiPixelRecHitSoAFromLegacy::produce(edm::StreamID streamID, edm::Event& iEv
assert(numberOfHits == numberOfClusters);

// fill data structure to support CA
for (auto i = 0; i < 11; ++i) {
for (auto i = 0U; i < phase1PixelTopology::numberOfLayers + 1; ++i) {
output->hitsLayerStart()[i] = hitsModuleStart[cpeView.layerGeometry().layerStart[i]];
}
cms::cuda::fillManyFromVector(
output->phiBinner(), 10, output->iphi(), output->hitsLayerStart(), numberOfHits, 256, nullptr);
cms::cuda::fillManyFromVector(output->phiBinner(),
phase1PixelTopology::numberOfLayers,
output->iphi(),
output->hitsLayerStart(),
numberOfHits,
256,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

256 should probably be a declared constant.

Copy link
Contributor Author

@czangela czangela Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but not just here. 256 as the number of threads is hardcoded in many places.
Should this be added as an issue to #32483 ? @fwyzard @mmusich @VinInn
edit: By "this" I meant making the number of threads / threadsPerBlock constants configurable, or setting them in one place? Also, if there were previous discussions about this, could you guide me to them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that this is a more general issue, that applies to all CUDA kernels, not just the Pixel ones

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When/if anybody will come with a good reason to make configurable it will be done. Until then no reason to name it.

nullptr);

LogDebug("SiPixelRecHitSoAFromLegacy") << "created HitSoa for " << numberOfClusters << " clusters in "
<< numberOfDetUnits << " Dets";
Expand Down