Apply fix from D81179 only from GCC < 8

[clementval]

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.

[isuruf]

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

[clementval]

In D81208#2075438, @isuruf wrote:

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]

In D81208#2076253, @clementval wrote:

In D81208#2075438, @isuruf wrote:

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

810

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

[DavidTruby]

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.

[clementval]

In D81208#2076450, @DavidTruby wrote:

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?

[DavidTruby]

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]

In D81208#2076548, @DavidTruby wrote:

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]

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.

[clementval]

Addresss comment

[clementval]

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

flang/include/flang/Evaluate/integer.h

public:
   constexpr Product MultiplyUnsigned(const Integer &y) const {
     Part product[2 * parts]{}; // little-endian full product
     for (int j{0}; j < parts; ++j) {
       if (Part xpart{LEPart(j)}) {
         for (int k{0}; k < parts; ++k) {
           if (Part ypart{y.LEPart(k)}) {
             BigPart xy{xpart};
             xy *= ypart;
-            // && to < (2 * parts) was added to avoid GCC < 8 build failure
-            // on -Werror=array-bounds
-            for (int to{ j + k }; xy != 0 && to < (2 * parts); ++to) {
+#if defined __GNUC__ && __GNUC__ < 8
+            // && to < (2 * parts) was added to avoid GCC < 8 build failure on
+            // -Werror=array-bounds. This can be removed if -Werror is disable.
+            for (int to{j + k}; xy != 0 && to < (2 * parts); ++to) {
+#else
+            for (int to{j + k}; xy != 0; ++to) {
+#endif
               xy += product[to];
               product[to] = xy & partMask;
               xy >>= partBits;
             }
           }
         }
       }
     }

flang/lib/Decimal/big-radix-floating-point.h

int RemoveLeastOrderZeroDigits() {
     int remove{0};
     if (digits_ > 0 && digit_[0] == 0) {
       while (remove < digits_ && digit_[remove] == 0) {
         ++remove;
       }
       if (remove >= digits_) {
         digits_ = 0;
       } else if (remove > 0) {
+#if defined __GNUC__ && __GNUC__ < 8
         // (&& j + remove < maxDigits) was added to avoid GCC < 8 build failure
-        // on -Werror=array-bounds
-        for (int j{ 0 }; j + remove < digits_ && (j + remove < maxDigits);
-             ++j) {
+        // on -Werror=array-bounds. This can be removed if -Werror is disable.
+        for (int j{0}; j + remove < digits_ && (j + remove < maxDigits); ++j) {
+#else
+        for (int j{0}; j + remove < digits_; ++j) {
+#endif
           digit_[j] = digit_[j + remove];
         }
         digits_ -= remove;
       }
     }
     return remove;
   }