Skip to content

2D Extension of 1D Cooley-Tukey FFT. Refactoring original not necesss…#1

Open
bertiethorpe wants to merge 1 commit intomarkp-gc:mainfrom
bertiethorpe:main
Open

2D Extension of 1D Cooley-Tukey FFT. Refactoring original not necesss…#1
bertiethorpe wants to merge 1 commit intomarkp-gc:mainfrom
bertiethorpe:main

Conversation

@bertiethorpe
Copy link
Copy Markdown

…ary as extension makes use of spatial variable separability.

Included also is bash script for performing sweeps for data analysis

…ary as extension makes use of spatial variable separability.

Included also is bash script for performing sweeps for data analysis
Copy link
Copy Markdown
Owner

@markp-gc markp-gc left a comment

Choose a reason for hiding this comment

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

Just need to avoid losing some of my recent chagnes: if you like I can merge just the new files (but then it won't credit you as the author).

// Can only do the second expression in-place:
auto tmpReal = popops::map(graph, complexMulExprRe, {real, v.real, imag, v.imag},
prog, debugPrefix + "/complex_mul_re");
popops::mapInPlace(graph, complexMulExprRe, {real, v.real, imag, v.imag},
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Need to keep my change here. Only the second operation can be done in place because it needs the input from the first.

poplar::Tensor partial =
poplin::matMul(graph, matrix.real, realBatch, prog,
elemType, debugStr + "/real_matmul", matmulOptions);
elemType, debugStr + "/real_matmul");
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Need to keep my change here (matmul options).


poplin::matMulAcc(graph, partial, 1.f, matrix.imag, imagBatch, prog,
debugStr + "/imag_matmul", matmulOptions);
debugStr + "/imag_matmul");
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Keep matmul options as above.

result_odd_remapped.mapLinearly(graph);
prog.add(copy(result_odd, result_odd_remapped));
result_odd = result_odd_remapped;
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Keep my change here: it uses a better layout for small FFTs to improve their performance and memory use.

auto result_odd_remapped = ComplexTensor(graph, result_even.elementType(), result_even.shape(), "dft_even_remapped");
result_odd_remapped.mapLinearly(graph);
prog.add(copy(result_odd, result_odd_remapped));
result_odd = result_odd_remapped;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Keep this as it improves memory use and perf.


ComplexTensor FFTBuilder::inverseFourierMatrices(
std::size_t length, poplar::Type elemType) {
const double twoPi_over_length = (2.0L / length) * 3.141592653589793238462643383279502884L;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Keep my change as it improves prexision significantly (same below).

availableMemoryProportion(-1.f), flopEstimate(0) {}

/// Set the proportion of memory available for the inner DFT matrix-multiplies.
void setAvailableMemoryProportion(float proportion) { availableMemoryProportion = proportion; }
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Keep availableMem prop changes.

@@ -0,0 +1,202 @@
// Copyright (c) 2022 Graphcore Ltd. All rights reserved.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If you like I can just merge the new files and leave everythign else unchaged?

@@ -0,0 +1,202 @@
// Copyright (c) 2022 Graphcore Ltd. All rights reserved.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Needs Graphcore copyright notice (copy format from one of the other files).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants