[Build] Resolved All IPO Warnings in VTR #3127
Conversation
github-actions
bot
added
libarchfpga
|
@AmirhosseinPoolad I recall that you had some comments about this PR, which is why I am holding on merging. To summarize our conversation, you were wondering why I only turned off the warning in the library that was throwing it and not just turn off the warning globally when IPO is set. This comes from my belief that its good practice to only ignore warnings that you are sure are moot. The idea is that if a new warning pops up in the future, it may actually be true and its important that we look into it (instead of instantly ignoring it). Thus, when I can, I like to only ignore warnings in fine-grain locations. After discussing with @vaughnbetz last week, it sounds like he does not care either way but does agree about hiding these warnings. There are other warnings in CapNProto which I cannot resolve in a fine-grain way and will have to globally turn off anyways. I am personally on the side of fine-grain turning off warnings when we can and only globally turning off warnings if we have to. What do you think? |
|
I actually thought we agreed to turn them off globally and I personally think that's better. I don't see any point in that specific link time warning and to be honest I don't see it ever giving any good warnings that aren't catched by the compiler beforehand. In addition, 100 erroneous warnings and 10 erroneous warnings have the same effect on people regarding ignoring warnings and the goal should be to have zero. |
I ain't gonna fight you on this, I am not married to it. You have a good point that we already have other checks that should catch this with IPO off. When IPO is on I believe there is still merit to knowing that its causing a problem; however, I recognize that not everyone thinks that way. I will update this PR and turn off all IPO warnings globally. |
VTR is built with IPO by default. IPO tends to reveal false warnings in the compiler. This was causing many warnings when VTR was built by default. Most of these warnings were coming from libarchfpga due to how it allocates arrays. Suppressed these warnings when IPO is turned on. There are still link-time warnings coming from CapNProto; however, since this library is external, it is a harder to supress these warnings. We could suppress the warnings by globally setting the link option to ignore the warning, but that may be overkill.
After discussing with other developers on VTR, decided to globally suppress all known IPO warnings in VTR.
AlexandreSinger
force-pushed
the
feature-build-ipo-warning-clean
branch
from
f604200 to
1e6de68
Compare
AlexandreSinger
changed the title
github-actions
bot
added
VPR
|
@AmirhosseinPoolad Resolved. Please review when you have a moment. |
There was a problem hiding this comment.
Thanks for the PR @AlexandreSinger, this would imo greatly enhance VTR developer experience. Other than the comment issue below, LGTM.
| # Suppress IPO link warnings. | ||
| # When IPO is turned on, it sometimes leads to false positives for warnings | ||
| # since it checks for warnings after some of the source files have been compiled. | ||
| # We globally suppress these warnings here. Any CMake executable which is added |
There was a problem hiding this comment.
I think you missed a "for" here
|
LGTM! Thanks. |
VTR is built with IPO by default. IPO tends to reveal false warnings in the compiler. This was causing many warnings when VTR was built by default.
Most of these warnings were coming from libarchfpga due to how it allocates arrays. Suppressed these warnings when IPO is turned on.
There are still link-time warnings coming from CapNProto; however, since this library is external, it is a harder to supress these warnings. We could suppress the warnings by globally setting the link option to ignore the warning, but that may be overkill.
Edit:
See discussion below, decided to globally suppress all known IPO warnings in VTR.