Closed npadmana closed 4 years ago
@ronawho -- care to review, and perf test? (The initial Travis failure was because it couldn't connect to dockerhub).
At a glance, this looks like a nice improvement to me. I can probably do performance runs on Thursday.
Coincidently I was thinking about how to clean up plan creation earlier today as I was learning about some new partial instantiation features in 1.20 ('partial instantiation' section of https://chapel-lang.org/releaseNotes/1.20/01-language.pdf)
With that, I was wondering if we could avoid some of the duplicate array and ft Type stuff and push the setup logic into an initializer. Something roughly like:
record FFTWplan {
type arrType;
param ftType: FFTtype;
...
proc init(dom: domain, numTransforms: int, signOrKind, flags : c_uint = FFTW_MEASURE) {
...
}
}
type Plan = FFTWplan(T, ftType);
var xPlan = new Plan({xDst, zSrc}, 1, signOrKind);
var yPlan = new Plan({ySrc, zSrc}, 1, signOrKind);
var zPlan = new Plan({zSrc}, 1, signOrKind);
But anyway, I think what's here is a nice cleanup, just some misc thoughts I had earlier today.
I’m not sure which way I’m leaning on this. Part of me likes the uniformity at the call site. But I agree it is a little odd to have the extra parameters for this case.
Maybe an overload for setupPlan?
On Tue, Oct 15, 2019 at 8:56 PM Elliot Ronaghan notifications@github.com wrote:
@ronawho commented on this pull request.
In src/DistributedFFT.chpl https://github.com/npadmana/DistributedFFT/pull/52#discussion_r335238756 :
@@ -214,9 +214,9 @@ prototype module DistributedFFT { const (yDst, xDst, _) = DstDom.localSubdomain().dims();
// Set up FFTW plans
- var xPlan = setup1DPlan(T, ftType, xDst.size, zSrc.size, signOrKind, FFTW_MEASURE);
- var yPlan = setup1DPlan(T, ftType, ySrc.size, zSrc.size, signOrKind, FFTW_MEASURE);
- var zPlan = setup1DPlan(T, ftType, zSrc.size, 1, signOrKind, FFTW_MEASURE);
- var xPlan = setupPlan(T, ftType, {xDst, zSrc}, parDim=2, 1, signOrKind, FFTW_MEASURE);
- var yPlan = setupPlan(T, ftType, {ySrc, zSrc}, parDim=2, 1, signOrKind, FFTW_MEASURE);
- var zPlan = setupPlan(T, ftType, {0..0, zSrc}, parDim=1, 1, signOrKind, FFTW_MEASURE);
I wonder if it would be cleaner to adjust setupPlan to look at the rank of the domain passed in, instead of adding 0..0. To me, this looks a little odd to be passing in parDim when the non-batched plan isn't responsible for the parallelization.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/npadmana/DistributedFFT/pull/52?email_source=notifications&email_token=AAER62SPVR7UATHGOG5ERTDQOZRDPA5CNFSM4JA746YKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCICFFNA#pullrequestreview-302273204, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAER62VDCRI4OITQJMMHVP3QOZRDPANCNFSM4JA746YA .
Maybe an overload for setupPlan?
Yeah, that might be nice.
FYI related to the initializer vs factory function style. I find https://github.com/chapel-lang/chapel/issues/14291 to be pretty clean for factory functions. (FFTWplan.{create,setup} vs. setupPlan)
I should be able to start perf testing around noon. Let me know if there's other changes you'd like to make before I start
I couldn't get into the queue today, but I'll try again tomorrow. To hold you over until then, today's xkcd cracked me up --https://xkcd.com/2216/
I see no performance changes for 128 and 256 node runs for size D/E/F
@ronawho -- thanks, and thanks for the perf test! I've been tied up with a bunch of other stuff these past two weeks, but will merge in later.
No rush on my account, I just saw you on some other threads and wanted to make sure you weren't waiting on me.
Fixes #51.
Tested on laptop --
Dist/test_simple
,NPB-FT/ft_transposed
(with class=S,W,A) andR2R/testr2r
all work. Also testedDist/test_simple
with the naive flag thrown.