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.

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

prov/efa: Get rid of noop instruction from empty #define #9654

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

a-szegel
Copy link
Contributor

@a-szegel a-szegel commented Dec 19, 2023

I was playing around on godbolt and saw that while (0) {} generates a noop instruction, but (void) 0; does not. Get rid of noop instructions from critical path without hurting the readability of the code.
Screenshot 2023-12-18 at 5 49 51 PM
Screenshot 2023-12-18 at 5 52 43 PM

@a-szegel a-szegel requested a review from a team December 19, 2023 02:04
@darrylabbate
Copy link
Member

  1. do {} while (0) (as some of the macros use) doesn't produce a nop. I'd prefer this over (void) 0.
  2. Optimizations aren't enabled, but even if the compiler is producing a nop, it's likely for optimization/scheduling/alignment purposes, i.e. minimum sizes for fetch/decode chunks. IOW, I'd be curious if nops are being emitted unconditionally or only in cases where the compiler has a good reason to do so.
  3. You should remove the trailing semicolons from the macro definitions.

I was playing around on godbolt and saw that while (0) {} generates a
noop instruction, but do {} while(0) does not.  Get rid of noop instructions
from critical path without hurting the readability of the code.

Signed-off-by: Seth Zegelstein <szegel@amazon.com>
@a-szegel a-szegel force-pushed the get-rid-of-noop-instruction branch from 490efce to 137d66c Compare December 19, 2023 18:20
@shijin-aws shijin-aws merged commit 6a7dba9 into ofiwg:main Dec 19, 2023
8 checks passed
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.

3 participants