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

Jump to bottom

UCT/IB/MLX5: fixed compilation issue #2353

Merged
merged 3 commits into from
Feb 28, 2018
Merged

UCT/IB/MLX5: fixed compilation issue #2353

merged 3 commits into from
Feb 28, 2018

Conversation

hoopoepg
Copy link
Contributor

  • fixed compilation on ARM platforms

UCT/IB/MLX5: fixed compilation issue
cf80d27 
- fixed compilation on ARM platforms
@swx-jenkins1
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/4103/ for details.

yosefe
yosefe reviewed Feb 27, 2018
View reviewed changes
src/uct/ib/mlx5/ib_mlx5.inl Outdated
n = wq->qend - dest;
memcpy(dest, src, n);
memcpy(wq->qstart, src + n, length - n);
if (length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like extra branch on fast-path? can't we silence the compiler somehow or make it only for ARM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

below there are conditions where length used, new condition should not affect to performance.

and original code is not valid: memcpy has unpredictable behavior on NULL ptr in zero-length.

so, look for ARM specific suppression?

@mellanox-github
Copy link
Contributor

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/6403/ for details (Mellanox internal link).

@yosefe
Copy link
Contributor

yosefe commented Feb 27, 2018

looks like gcc has hard time to propagate constants, this happens from uct_rc/dc_mlx5_ep_tag_eager_zcopy() only. where the inline copy function is not called.
so can pass some non-NULL pointer as "am_header" just to silence to compiler, for example "iface", "ep", etc.

@swx-jenkins1
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/4104/ for details.

@mellanox-github
Copy link
Contributor

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/6404/ for details (Mellanox internal link).

@swx-jenkins1
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/4106/ for details.

@yosefe yosefe added the Bugfix label Feb 28, 2018
@hoopoepg
Copy link
Contributor Author

bot:mlx:retest

@mellanox-github
Copy link
Contributor

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/6406/ for details (Mellanox internal link).

@mellanox-github
Copy link
Contributor

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/6408/ for details (Mellanox internal link).

@hoopoepg
Copy link
Contributor Author

@yosefe tests are passed

@yosefe yosefe merged commit a7dce0b into openucx:master Feb 28, 2018
@hoopoepg hoopoepg mentioned this pull request Mar 7, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yosefe yosefe yosefe approved these changes

Assignees
No one assigned
Labels
Bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hoopoepg @swx-jenkins1 @mellanox-github @yosefe