This is an archive of the discontinued LLVM Phabricator instance.

Apply fix from D81179 only from GCC < 8
ClosedPublic

Authored by clementval on Jun 4 2020, 6:48 PM.

Details

Summary

Apply workaround done in D81179 only for GCC < 8. As @klausler mentioned in D81179 we want to avoid additional checks for other compilers that do not need them.

Diff Detail

Event Timeline

clementval created this revision.Jun 4 2020, 6:48 PM
isuruf added a subscriber: isuruf.Jun 4 2020, 11:14 PM

Shouldn't the solution be to suppress the warning by adding -Wno-array-bounds if GCC < 8? Or do something like

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Warray-bounds"
...
#pragma GCC diagnostic pop

Shouldn't the solution be to suppress the warning by adding -Wno-array-bounds if GCC < 8? Or do something like

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Warray-bounds"
...
#pragma GCC diagnostic pop

I don't think we want to disable the warnings for all files in Flang. I tried the pragma solution but it doesn't work in this case. Error are still triggered even the -Warray-bounds warning should be ignored.

tskeith added a subscriber: tskeith.Jun 5 2020, 7:14 AM

Shouldn't the solution be to suppress the warning by adding -Wno-array-bounds if GCC < 8? ...

I don't think we want to disable the warnings for all files in Flang.

In my opinion it would be okay to disable that warning for all of flang when compiling with GCC 7. The warning has proven to be unreliable and the valid warning situations can still be discovered by other compilers. The only risk is that someone developing with GCC 7 might introduce code that triggers a valid instance of this warning and it would go undetected until built with other compilers.

The advantage of having it in cmake code is that it doesn't clutter the C++ could with work-arounds.

Looking at examples in llvm, something like this might work:

if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 8.0)
  append("-Wno-array-bounds" CMAKE_CXX_FLAGS)
endif()
flang/include/flang/Evaluate/integer.h
813

Please be sure to run clang-format on the changes.

sscalpone accepted this revision.Jun 5 2020, 7:52 AM
This revision is now accepted and ready to land.Jun 5 2020, 7:52 AM

Will there actually be a performance issue just leaving the code without disabling for other compilers? If other compilers weren't issuing this warning then they by definition know that the bounds check isn't necessary and therefore can optimize it out I'd have thought.

Will there actually be a performance issue just leaving the code without disabling for other compilers? If other compilers weren't issuing this warning then they by definition know that the bounds check isn't necessary and therefore can optimize it out I'd have thought.

Yeah it could be the case. Should we maybe keep the workaround as in D81179 and just add a comment in case one performance problem comes out in the future?

I would prefer it that way but I don't really have strong opinions on it so don't consider me blocking or anything.

clementval added a comment.EditedJun 8 2020, 11:01 AM

I would prefer it that way but I don't really have strong opinions on it so don't consider me blocking or anything.

@klausler Are you ok with this fix?

klausler requested changes to this revision.Jun 8 2020, 11:20 AM
klausler added inline comments.
flang/lib/Decimal/big-radix-floating-point.h
182

If compiling with other than G++, GNUC won't necessarily be defined, and will be replaced with 0, and the needless work-around will be enabled.

I suggest #if defined __GNUC__ && __GNUC__ < 8.

Please also use clang-format to make the code consistent; thanks.

This revision now requires changes to proceed.Jun 8 2020, 11:20 AM
clementval edited the summary of this revision. (Show Details)

Addresss comment

Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 12:15 PM
clementval marked 2 inline comments as done.Jun 8 2020, 12:17 PM

@klausler I had an outdated clang-format so that's why I had a bad formatting. Sorry for that. This should be fine now.

klausler accepted this revision.Jun 8 2020, 12:46 PM
This revision is now accepted and ready to land.Jun 8 2020, 12:46 PM
This revision was automatically updated to reflect the committed changes.