eliminating g++ warnings #105520

fschlimb · 2024-08-21T13:12:56Z

Eliminating g++ warnings. Mostly declaring "[[maybe_unused]]", adding return statements where missing and fixing casts.

llvmbot · 2024-08-21T13:13:28Z

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-mlir
@llvm/pr-subscribers-mlir-index
@llvm/pr-subscribers-mlir-amdgpu
@llvm/pr-subscribers-mlir-gpu
@llvm/pr-subscribers-mlir-sme
@llvm/pr-subscribers-mlir-async
@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-mlir-spirv

Author: Frank Schlimbach (fschlimb)

Changes

Eliminating g++ warnings. Mostly declaring "[[maybe_unused]]", adding return statements where missing and fixing casts.

@rengolin

Full diff: https://github.com/llvm/llvm-project/pull/105520.diff

15 Files Affected:

(modified) llvm/include/llvm/BinaryFormat/MachO.h (+1-1)
(modified) mlir/include/mlir/IR/OpDefinition.h (+4-3)
(modified) mlir/include/mlir/Pass/Pass.h (+1)
(modified) mlir/include/mlir/Query/Matcher/Marshallers.h (+4-5)
(modified) mlir/lib/Analysis/FlatLinearValueConstraints.cpp (+1-1)
(modified) mlir/lib/CAPI/IR/IR.cpp (+2)
(modified) mlir/lib/Conversion/ArmSMEToLLVM/ArmSMEToLLVM.cpp (+6)
(modified) mlir/lib/Conversion/IndexToSPIRV/IndexToSPIRV.cpp (+2)
(modified) mlir/lib/Debug/DebuggerExecutionContextHook.cpp (+2-2)
(modified) mlir/lib/Dialect/AMDGPU/IR/AMDGPUDialect.cpp (+1-1)
(modified) mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp (+2)
(modified) mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp (+1)
(modified) mlir/lib/Dialect/Index/IR/IndexOps.cpp (+2)
(modified) mlir/unittests/Bytecode/BytecodeTest.cpp (+1-1)
(modified) mlir/unittests/Support/CyclicReplacerCacheTest.cpp (+2-1)

diff --git a/llvm/include/llvm/BinaryFormat/MachO.h b/llvm/include/llvm/BinaryFormat/MachO.h
index b37651e85a6269..17f30d9d0d0b23 100644
--- a/llvm/include/llvm/BinaryFormat/MachO.h
+++ b/llvm/include/llvm/BinaryFormat/MachO.h
@@ -1662,7 +1662,7 @@ CPU_SUBTYPE_ARM64E_WITH_PTRAUTH_VERSION(unsigned PtrAuthABIVersion,
          "ptrauth abi version must fit in 4 bits");
   return CPU_SUBTYPE_ARM64E | CPU_SUBTYPE_ARM64E_VERSIONED_PTRAUTH_ABI_MASK |
          (PtrAuthKernelABIVersion ? CPU_SUBTYPE_ARM64E_KERNEL_PTRAUTH_ABI_MASK
-                                  : 0) |
+                                  : static_cast<CPUSubTypeARM64>(0)) |
          (PtrAuthABIVersion << 24);
 }
 
diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h
index 59f094d6690991..1121de858aa632 100644
--- a/mlir/include/mlir/IR/OpDefinition.h
+++ b/mlir/include/mlir/IR/OpDefinition.h
@@ -1593,7 +1593,8 @@ foldTrait(Operation *, ArrayRef<Attribute>, SmallVectorImpl<OpFoldResult> &) {
 /// Given a tuple type containing a set of traits, return the result of folding
 /// the given operation.
 template <typename... Ts>
-static LogicalResult foldTraits(Operation *op, ArrayRef<Attribute> operands,
+static LogicalResult foldTraits([[maybe_unused]] Operation *op,
+                                [[maybe_unused]] ArrayRef<Attribute> operands,
                                 SmallVectorImpl<OpFoldResult> &results) {
   return success((succeeded(foldTrait<Ts>(op, operands, results)) || ...));
 }
@@ -1629,7 +1630,7 @@ verifyTrait(Operation *) {
 
 /// Given a set of traits, return the result of verifying the given operation.
 template <typename... Ts>
-LogicalResult verifyTraits(Operation *op) {
+LogicalResult verifyTraits([[maybe_unused]] Operation *op) {
   return success((succeeded(verifyTrait<Ts>(op)) && ...));
 }
 
@@ -1649,7 +1650,7 @@ verifyRegionTrait(Operation *) {
 /// Given a set of traits, return the result of verifying the regions of the
 /// given operation.
 template <typename... Ts>
-LogicalResult verifyRegionTraits(Operation *op) {
+LogicalResult verifyRegionTraits([[maybe_unused]] Operation *op) {
   return success((succeeded(verifyRegionTrait<Ts>(op)) && ...));
 }
 } // namespace op_definition_impl
diff --git a/mlir/include/mlir/Pass/Pass.h b/mlir/include/mlir/Pass/Pass.h
index 7725a3a2910bd4..e7ed0a1ac61395 100644
--- a/mlir/include/mlir/Pass/Pass.h
+++ b/mlir/include/mlir/Pass/Pass.h
@@ -94,6 +94,7 @@ class Pass {
     Option(Pass &parent, StringRef arg, Args &&...args)
         : detail::PassOptions::Option<DataType, OptionParser>(
               parent.passOptions, arg, std::forward<Args>(args)...) {}
+    Option &operator=(const Option &other) = default; // gcc11
     using detail::PassOptions::Option<DataType, OptionParser>::operator=;
   };
   /// This class represents a specific pass option that contains a list of
diff --git a/mlir/include/mlir/Query/Matcher/Marshallers.h b/mlir/include/mlir/Query/Matcher/Marshallers.h
index 6ed35ac0ddccc7..d230df042aed04 100644
--- a/mlir/include/mlir/Query/Matcher/Marshallers.h
+++ b/mlir/include/mlir/Query/Matcher/Marshallers.h
@@ -150,11 +150,10 @@ inline bool checkArgTypeAtIndex(llvm::StringRef matcherName,
 
 // Marshaller function for fixed number of arguments
 template <typename ReturnType, typename... ArgTypes, size_t... Is>
-static VariantMatcher
-matcherMarshallFixedImpl(void (*matcherFunc)(), llvm::StringRef matcherName,
-                         SourceRange nameRange,
-                         llvm::ArrayRef<ParserValue> args, Diagnostics *error,
-                         std::index_sequence<Is...>) {
+static VariantMatcher matcherMarshallFixedImpl(
+    void (*matcherFunc)(), [[maybe_unused]] llvm::StringRef matcherName,
+    SourceRange nameRange, llvm::ArrayRef<ParserValue> args, Diagnostics *error,
+    std::index_sequence<Is...>) {
   using FuncType = ReturnType (*)(ArgTypes...);
 
   // Check if the argument count matches the expected count
diff --git a/mlir/lib/Analysis/FlatLinearValueConstraints.cpp b/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
index e628fb152b52f8..6778029bb5ca64 100644
--- a/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
+++ b/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
@@ -893,7 +893,7 @@ FlatLinearValueConstraints::FlatLinearValueConstraints(IntegerSet set,
                             set.getNumDims(), set.getNumSymbols(),
                             /*numLocals=*/0) {
   assert(operands.empty() ||
-         set.getNumInputs() == operands.size() && "operand count mismatch");
+         (set.getNumInputs() == operands.size() && "operand count mismatch"));
   // Set the values for the non-local variables.
   for (unsigned i = 0, e = operands.size(); i < e; ++i)
     setValue(i, operands[i]);
diff --git a/mlir/lib/CAPI/IR/IR.cpp b/mlir/lib/CAPI/IR/IR.cpp
index 5eb531b70aee05..eb057bcc9d5e56 100644
--- a/mlir/lib/CAPI/IR/IR.cpp
+++ b/mlir/lib/CAPI/IR/IR.cpp
@@ -736,6 +736,8 @@ static mlir::WalkResult unwrap(MlirWalkResult result) {
   case MlirWalkResultSkip:
     return mlir::WalkResult::skip();
   }
+  assert("unknown result in WalkResult::unwrap" == nullptr);
+  return {};
 }
 
 void mlirOperationWalk(MlirOperation op, MlirOperationWalkCallback callback,
diff --git a/mlir/lib/Conversion/ArmSMEToLLVM/ArmSMEToLLVM.cpp b/mlir/lib/Conversion/ArmSMEToLLVM/ArmSMEToLLVM.cpp
index 4d96091a637cf0..c44037bccd444a 100644
--- a/mlir/lib/Conversion/ArmSMEToLLVM/ArmSMEToLLVM.cpp
+++ b/mlir/lib/Conversion/ArmSMEToLLVM/ArmSMEToLLVM.cpp
@@ -80,6 +80,8 @@ static Operation *createLoadTileSliceIntrinsic(
       break;
     }
   }
+  assert("unknown type in createLoadTileSliceIntrinsic" == nullptr);
+  return nullptr;
 }
 
 /// Helper to create an arm_sme.intr.st1*.(horiz|vert)' intrinsic.
@@ -124,6 +126,8 @@ static Operation *createStoreTileSliceIntrinsic(
           loc, maskOp, ptr, tileId, tileSliceI32);
     }
   }
+  assert("unknown type in createStoreTileSliceIntrinsic" == nullptr);
+  return nullptr;
 }
 
 IntegerAttr getTileIdOrError(arm_sme::ArmSMETileOpInterface op) {
@@ -848,6 +852,8 @@ struct StreamingVLOpConversion
       case arm_sme::TypeSize::Double:
         return rewriter.create<arm_sme::aarch64_sme_cntsd>(loc, i64Type);
       }
+      assert("unknown type in StreamingVLOpConversion" == nullptr);
+      return nullptr;
     }();
     rewriter.replaceOpWithNewOp<arith::IndexCastOp>(
         streamingVlOp, rewriter.getIndexType(), intrOp->getResult(0));
diff --git a/mlir/lib/Conversion/IndexToSPIRV/IndexToSPIRV.cpp b/mlir/lib/Conversion/IndexToSPIRV/IndexToSPIRV.cpp
index b58efc096e2eaf..30705a23d5bb6f 100644
--- a/mlir/lib/Conversion/IndexToSPIRV/IndexToSPIRV.cpp
+++ b/mlir/lib/Conversion/IndexToSPIRV/IndexToSPIRV.cpp
@@ -310,6 +310,8 @@ struct ConvertIndexCmpPattern final : OpConversionPattern<CmpOp> {
     case IndexCmpPredicate::ULT:
       return rewriteCmpOp<spirv::ULessThanOp>(op, adaptor, rewriter);
     }
+    assert("Unknown predicate in ConvertIndexCmpPattern" == nullptr);
+    return failure();
   }
 };
 
diff --git a/mlir/lib/Debug/DebuggerExecutionContextHook.cpp b/mlir/lib/Debug/DebuggerExecutionContextHook.cpp
index 744a0380ec710b..342888cc9cbcbe 100644
--- a/mlir/lib/Debug/DebuggerExecutionContextHook.cpp
+++ b/mlir/lib/Debug/DebuggerExecutionContextHook.cpp
@@ -301,7 +301,7 @@ void mlirDebuggerAddFileLineColLocBreakpoint(const char *file, int line,
 
 LLVM_ATTRIBUTE_NOINLINE void mlirDebuggerBreakpointHook() {
   static LLVM_THREAD_LOCAL void *volatile sink;
-  sink = (void *)&sink;
+  sink = (void *)const_cast<void **>(&sink);
 }
 
 static void preventLinkerDeadCodeElim() {
@@ -321,7 +321,7 @@ static void preventLinkerDeadCodeElim() {
     sink = (void *)mlirDebuggerAddTagBreakpoint;
     sink = (void *)mlirDebuggerAddRewritePatternBreakpoint;
     sink = (void *)mlirDebuggerAddFileLineColLocBreakpoint;
-    sink = (void *)&sink;
+    sink = (void *)const_cast<void **>(&sink);
     return true;
   }();
   (void)initialized;
diff --git a/mlir/lib/Dialect/AMDGPU/IR/AMDGPUDialect.cpp b/mlir/lib/Dialect/AMDGPU/IR/AMDGPUDialect.cpp
index c1a785fb25478d..85ea49bed1dd0d 100644
--- a/mlir/lib/Dialect/AMDGPU/IR/AMDGPUDialect.cpp
+++ b/mlir/lib/Dialect/AMDGPU/IR/AMDGPUDialect.cpp
@@ -347,7 +347,7 @@ LogicalResult DPPOp::verify() {
       return emitOpError("quad_perm attribute must have exactly 4 elements");
     }
     for (auto elem : quadPermAttr.getAsRange<IntegerAttr>()) {
-      uint32_t num = elem.getInt();
+      int32_t num = elem.getInt();
       if (num < 0 || num > 3) {
         return emitOpError(
             "Each element of quad_perm must be in the range [0, 3]");
diff --git a/mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp b/mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp
index 3a2042d23e5346..1ec0e3cd48863f 100644
--- a/mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp
+++ b/mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp
@@ -137,6 +137,8 @@ static ArrayRef<TileMask> getMasks(ArmSMETileType type) {
   case ArmSMETileType::ZAQ:
     return ZA_Q_MASKS;
   }
+  assert("unknown type in getMasks" == nullptr);
+  return {};
 }
 
 class TileAllocator {
diff --git a/mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp b/mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp
index 8c3e25355f6087..1db38232e77fe7 100644
--- a/mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp
+++ b/mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp
@@ -175,6 +175,7 @@ ArrayRef<BlockArgument> ParallelComputeFunctionArgs::lowerBounds() {
   return args.drop_front(2 + 1 * numLoops).take_front(numLoops);
 }
 
+[[maybe_unused]]
 ArrayRef<BlockArgument> ParallelComputeFunctionArgs::upperBounds() {
   return args.drop_front(2 + 2 * numLoops).take_front(numLoops);
 }
diff --git a/mlir/lib/Dialect/Index/IR/IndexOps.cpp b/mlir/lib/Dialect/Index/IR/IndexOps.cpp
index 42401dae217ce1..cb5f48f2d603ad 100644
--- a/mlir/lib/Dialect/Index/IR/IndexOps.cpp
+++ b/mlir/lib/Dialect/Index/IR/IndexOps.cpp
@@ -594,6 +594,8 @@ static bool compareSameArgs(IndexCmpPredicate pred) {
   case IndexCmpPredicate::ULT:
     return false;
   }
+  assert("unknown predicate in compareSameArgs" == nullptr);
+  return {};
 }
 
 OpFoldResult CmpOp::fold(FoldAdaptor adaptor) {
diff --git a/mlir/unittests/Bytecode/BytecodeTest.cpp b/mlir/unittests/Bytecode/BytecodeTest.cpp
index a37a2afc226453..5f1f8bfd2e1758 100644
--- a/mlir/unittests/Bytecode/BytecodeTest.cpp
+++ b/mlir/unittests/Bytecode/BytecodeTest.cpp
@@ -55,7 +55,7 @@ TEST(Bytecode, MultiModuleWithResource) {
   constexpr size_t kAlignment = 0x20;
   size_t bufferSize = buffer.size();
   buffer.reserve(bufferSize + kAlignment - 1);
-  size_t pad = ~(uintptr_t)buffer.data() + 1 & kAlignment - 1;
+  size_t pad = (~(uintptr_t)buffer.data() + 1) & (kAlignment - 1);
   buffer.insert(0, pad, ' ');
   StringRef alignedBuffer(buffer.data() + pad, bufferSize);
 
diff --git a/mlir/unittests/Support/CyclicReplacerCacheTest.cpp b/mlir/unittests/Support/CyclicReplacerCacheTest.cpp
index 64a8ab72b69b7d..26f0709f7d8310 100644
--- a/mlir/unittests/Support/CyclicReplacerCacheTest.cpp
+++ b/mlir/unittests/Support/CyclicReplacerCacheTest.cpp
@@ -225,7 +225,8 @@ class CachedCyclicReplacerGraphReplacement : public ::testing::Test {
     /// Add a recursive-self-node, i.e. a duplicate of the original node that is
     /// meant to represent an indirection to it.
     std::pair<Node, int64_t> addRecursiveSelfNode(Graph::Node originalId) {
-      return {addNode(originalId, nextRecursionId), nextRecursionId++};
+      auto node = addNode(originalId, nextRecursionId);
+      return {node, nextRecursionId++};
     }
     void addEdge(Node src, Node sink) { connections.addEdge(src, sink); }

mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp

kuhar · 2024-08-21T13:46:52Z

mlir/include/mlir/IR/OpDefinition.h

+static LogicalResult foldTraits([[maybe_unused]] Operation *op,
+                                [[maybe_unused]] ArrayRef<Attribute> operands,


This and the other ones below seem clearly used unless sizeof...(Ts) is 0. Would adding a static assert potentially fix that?
Otherwise the negative of declaring these as maybe unused has the downside that if they truly become unused at some point in the future, we won't get that warning

static_assert(sizeof(op) && sizeof(operands)); // avoid "unused" warning
also avoids the compiler message.
Would that be acceptable?

Well, this would probably not address your concern, though.

I could offer the following, which might be a bit too much for the gain:

template <typename... Ts, std::size_t _i = sizeof...(Ts), std::enable_if_t<(_i > 0), int> = 0> static LogicalResult foldTraits(Operation *op, ArrayRef<Attribute> operands, SmallVectorImpl<OpFoldResult> &results) { return success((succeeded(foldTrait<Ts>(op, operands, results)) || ...)); } // this specialization avoid compiler warnings about unused params template <typename... Ts, std::size_t _i = sizeof...(Ts), std::enable_if_t<(_i == 0), int> = 0> static LogicalResult foldTraits([[maybe_unused]] Operation *op, [[maybe_unused]] ArrayRef<Attribute> operands, SmallVectorImpl<OpFoldResult> &results) { return success((succeeded(foldTrait<Ts>(op, operands, results)) || ...)); }

another workaround is to add a base case and then the rest of the parameter pack:

template <typename First, typename... Rest> static LogicalResult foldTraits(Operation *op, ArrayRef<Attribute> operands, SmallVectorImpl<OpFoldResult> &results) { return success((succeeded(foldTrait<First>(op, operands, results) || succeeded(foldTrait<Rest>(op, operands, results)) || ...)); }

@dwblaikie what do you think about this?

in particular since the warning does not appear when I add an actually unused parameter.

I'm not sure what's meant by this ^ - that the unused parameter warning (in GCC or Clang?) doesn't work/fire correctly on something genuinely/totally unused, but for GCC it /does/ fire on something unused only when the pack is zero sized? That's super confusing.

Even with that aside, I'd say if GCC's warning has a false positive (warning on an "unused" variable when the pack expands to zero elements) - we should disable the GCC warning. We do this for other GCC warnings that have false positives compared to clang.

If it's been fixed in some version of GCC, we can conditionally enable (or conditionally disable) it, either based on the behavior of the warning (again, something we do when bugs get fixed in warnings) in cmake with a snippet of code to test the compiler behavior - or based on compiler version, I think, probably.

I believe we already disable this warning (is it this one?) with some known version of GCC that have false positives: https://github.com/llvm/llvm-project/blob/main/mlir/CMakeLists.txt#L87-L94

It would be nice to clarify here what are we working around: is it the same warning? Which version of GCC?

Yes, it's the same warning (-Wunused-but-set-parameter) as handled in the CMakeLists.txt that you referred to. The warning triggers for gcc 11, 12, and 13 (didn't try other versions).

What's so bad about using maybe_unused? It works around single instance (in a very small function) of a questionable trigger of a warning. In my mind, disabling a warning for the entire project potentially does more harm.

Sure, I can disable the warning if that's preferred. I can simply adjust the version check. Let me know.

Ok, I disabled the warning for gcc<14 and removed my previously added [[maybe_unused]] in related places.

kuhar · 2024-08-21T13:47:19Z

mlir/include/mlir/Query/Matcher/Marshallers.h

+static VariantMatcher matcherMarshallFixedImpl(
+    void (*matcherFunc)(), [[maybe_unused]] llvm::StringRef matcherName,
+    SourceRange nameRange, llvm::ArrayRef<ParserValue> args, Diagnostics *error,
+    std::index_sequence<Is...>) {


disabled compiler warning

mlir/lib/Dialect/Index/IR/IndexOps.cpp

mlir/lib/Conversion/ArmSMEToLLVM/ArmSMEToLLVM.cpp

MacDue

Arm related changes LGTM, though wait for others to look at the rest of the changes 🙂

dwblaikie · 2024-08-21T17:22:22Z

mlir/include/mlir/Pass/Pass.h

@@ -94,6 +94,7 @@ class Pass {
    Option(Pass &parent, StringRef arg, Args &&...args)
        : detail::PassOptions::Option<DataType, OptionParser>(
              parent.passOptions, arg, std::forward<Args>(args)...) {}
+    Option &operator=(const Option &other) = default; // gcc11


what's the problem here? What warning/issue is being addressed? (& the comment could probably be more verbose/explicit)

This gets triggered by g++-11 -Wdeprecated-copy.
Here's the message:

llvm-project/mlir/test/lib/Analysis/DataFlow/TestDenseBackwardDataFlowAnalysis.cpp: In copy constructor ‘{anonymous}::TestNextAccessPass::TestNextAccessPass(const {anonymous}::TestNextAccessPass&)’: llvm-project/mlir/test/lib/Analysis/DataFlow/TestDenseBackwardDataFlowAnalysis.cpp:194:29: warning: implicitly-declared ‘mlir::Pass::Option<bool>& mlir::Pass::Option<bool>::operator=(const mlir::Pass::Option<bool>&)’ is deprecated [-Wdeprecated-copy] 194 | interprocedural = other.interprocedural; | ^~~~~~~~~~~~~~~ In file included from llvm-project/mlir/include/mlir/Pass/PassRegistry.h:17, from llvm-project/mlir/include/mlir/Pass/Pass.h:14, from llvm-project/mlir/test/lib/Analysis/DataFlow/TestDenseBackwardDataFlowAnalysis.cpp:25: llvm-project/mlir/include/mlir/Pass/PassOptions.h:213:13: note: because ‘mlir::Pass::Option<bool>’ has user-provided ‘mlir::detail::PassOptions::Option<DataType, OptionParser>& mlir::detail::PassOptions::Option<DataType, OptionParser>::operator=(const mlir::detail::PassOptions::Option<DataType, OptionParser>&) [with DataType = bool; OptionParser = llvm::cl::parser<bool>]’ 213 | Option &operator=(const Option &other) {

g++-12 does not report this. I am not entirely sure, but this might actually be a false-positive in g++-11.

If it's a false positive, please disable the warning under the false positive case/situation (either by compiler version, or compiler capability test that demonstrates the false positive and disables the warning if the false positive is observed (but probably worth documenting the compiler version we know have the false positive in that case anyway, so we know when to remove the checking))

dwblaikie · 2024-08-21T17:23:37Z

mlir/lib/Analysis/FlatLinearValueConstraints.cpp

@@ -893,7 +893,7 @@ FlatLinearValueConstraints::FlatLinearValueConstraints(IntegerSet set,
                            set.getNumDims(), set.getNumSymbols(),
                            /*numLocals=*/0) {
  assert(operands.empty() ||
-         set.getNumInputs() == operands.size() && "operand count mismatch");
+         (set.getNumInputs() == operands.size() && "operand count mismatch"));


Seems like the wrong bracing - though, again, this one comes up semi-regularly and I'd consider disabling the GCC warning, since Clang's does the right thing by observing that the RHS is a constant true and so the braces don't change the expression semantics here)

If we're adding braces, though, it should probably be around the whole assertion condition:

assert((operands.empty() || set.getNumInputs() == operands.size()) && "...");

Yes, the point here is that the braces should not change the semantics. gcc "suggests" braces to clarify. I tried to add braces without changing semantics. What you are proposing looks more correct, but is not the same as without.

Personally, I've always been a fan of over-bracing instead of under-bracing, especially in cases where compilers have "doubts".

IIUC, the GGC warning isn't because it doesn't know what to do, but that it recognises that this is a common bug pattern that people don't usually get the semantics right.

Adding the braces in the right place would make it easier for humans to read and all agree on the semantics, which is a good strategy regardless of warnings.

Edited to add: In the particular case of assert, I always add braces as @dwblaikie shows when there's more than one check.

What you are proposing looks more correct, but is not the same as without.

It is the same semantics (though it gets there through a different intermediate path) because string literals are always true. and (x || y) && true is the same as x || (y && true) - and the former is more what people intend, and means line wrapping/formatting will align with what the user intent, etc.

It's not that GCC doesn't know what to do - and it is a common bug pattern in general, and clang warns on this case in general - but clang has a carveout for string literals (& maybe constants more generally) because in this particular context it's not bug prone - the string literal being always true makes the bug benign.

Adjusted braces.

dwblaikie · 2024-08-21T17:24:25Z

mlir/lib/Debug/DebuggerExecutionContextHook.cpp

@@ -301,7 +301,7 @@ void mlirDebuggerAddFileLineColLocBreakpoint(const char *file, int line,

 LLVM_ATTRIBUTE_NOINLINE void mlirDebuggerBreakpointHook() {
  static LLVM_THREAD_LOCAL void *volatile sink;
-  sink = (void *)&sink;
+  sink = (void *)const_cast<void **>(&sink);


What warning or error is being worked around here?

casting away volatile discards qualifiers.

A warning? Again, could ignore the warning if clang doesn't catch it and it doesn't seem to be adding value?

Though technically the LLVM style guide does say to prefer C++ style casts over C style ones, I think? So perhaps we could lean into that and replace this with two C++ casts, rather than a C and a C++ one?

changed to using C++ casts only

dwblaikie · 2024-08-21T17:26:43Z

mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp

@@ -175,6 +175,7 @@ ArrayRef<BlockArgument> ParallelComputeFunctionArgs::lowerBounds() {
  return args.drop_front(2 + 1 * numLoops).take_front(numLoops);
 }

+[[maybe_unused]]


Looks like this code is dead and should be removed?

Fine with me, let me know if this should be removed.

github-actions · 2024-09-03T08:00:54Z

✅ With the latest revision this PR passed the C/C++ code formatter.

fschlimb · 2024-09-03T08:01:21Z

Rebased.
@dwblaikie @arsenm @joker-eph @kuhar Any other comments? Can this be merged?

kuhar · 2024-09-03T13:57:38Z

Rebased. @dwblaikie @arsenm @joker-eph @kuhar Any other comments? Can this be merged?

I can still see the [[maybe_unused]] attributes in #105520 (comment) and #105520 (comment). I thought we wouldn't need them after the cmake change?

fschlimb · 2024-09-26T09:41:32Z

Any other comments? Can this be merged?

joker-eph

LGTM!

fschlimb · 2024-10-15T09:41:49Z

Please, could anyone with write access merge this?

Co-authored-by: Benjamin Maxwell <[email protected]>

…ated cases

rengolin · 2024-10-18T20:20:35Z

Tests passed, there was a merge, which I resolved. The windows failures are unrelated.

fschlimb requested review from Mogball, banach-space, dcaballe and nicolasvasilache as code owners August 21, 2024 13:12

llvmbot added backend:AMDGPU mlir:core MLIR Core Infrastructure mlir:gpu mlir:spirv mlir mlir:amdgpu mlir:sme mlir:async mlir:index llvm:binary-utilities labels Aug 21, 2024

MacDue reviewed Aug 21, 2024

View reviewed changes

mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp Outdated Show resolved Hide resolved

rengolin requested a review from AaronBallman August 21, 2024 13:17

kuhar reviewed Aug 21, 2024

View reviewed changes

arsenm reviewed Aug 21, 2024

View reviewed changes

mlir/lib/Dialect/Index/IR/IndexOps.cpp Outdated Show resolved Hide resolved

fschlimb force-pushed the warn_gcc11 branch from 2da8232 to 72d3835 Compare August 21, 2024 16:54

MacDue reviewed Aug 21, 2024

View reviewed changes

mlir/lib/Conversion/ArmSMEToLLVM/ArmSMEToLLVM.cpp Outdated Show resolved Hide resolved

MacDue approved these changes Aug 21, 2024

View reviewed changes

kuhar requested a review from dwblaikie August 21, 2024 17:13

dwblaikie reviewed Aug 21, 2024

View reviewed changes

fschlimb force-pushed the warn_gcc11 branch from c93ac59 to d4cdeeb Compare September 3, 2024 07:57

llvmbot added debuginfo llvm:ir labels Sep 3, 2024

joker-eph approved these changes Sep 26, 2024

View reviewed changes

fschlimb force-pushed the warn_gcc11 branch from ae2283c to fe118f5 Compare September 27, 2024 07:44

fschlimb requested a review from hanhanW as a code owner September 27, 2024 07:44

fschlimb force-pushed the warn_gcc11 branch from fe118f5 to 9308d5b Compare September 27, 2024 13:54

fschlimb force-pushed the warn_gcc11 branch from 9308d5b to a3fe7c4 Compare October 14, 2024 16:31

fschlimb force-pushed the warn_gcc11 branch 3 times, most recently from 1f79bdf to 9d8f8e1 Compare October 18, 2024 12:45

fschlimb and others added 13 commits October 18, 2024 15:07

eliminating g++ warnings

37369b5

using llvm_unreachable

2dffbcf

Update mlir/lib/Conversion/ArmSMEToLLVM/ArmSMEToLLVM.cpp

8846cae

Co-authored-by: Benjamin Maxwell <[email protected]>

fixed braces

dd74d94

(void *) -> static_cast<void *>(...)

00da58e

-Wno-unused-but-set-parameter for gcc<14; no [[maybe-unused]] for rel…

3f82636

…ated cases

rm [[maybe_unused]]

5b06494

rm [[maybe_unused]]

6d5d7b6

removing unused ParallelComputeFunctionArgs::upperBounds

b18b50a

removing unused ParallelComputeFunctionArgs::upperBounds

218f354

silencing gcc false positive warning, rebased

9b104e2

static_cast

fe8b153

rebasing

cdd7464

fschlimb force-pushed the warn_gcc11 branch from 9d8f8e1 to cdd7464 Compare October 18, 2024 13:21

fschlimb and others added 2 commits October 18, 2024 17:35

Merge branch 'main' into warn_gcc11

6054e68

Merge branch 'main' into warn_gcc11

8697353

rengolin merged commit d5746d7 into llvm:main Oct 18, 2024
4 of 5 checks passed

jeremy-rifkin mentioned this pull request Jan 16, 2025

Clang does not warn about missing returns when a switch statement covers all enum values #123153

Open

		static LogicalResult foldTraits([[maybe_unused]] Operation *op,
		[[maybe_unused]] ArrayRef<Attribute> operands,

eliminating g++ warnings #105520

eliminating g++ warnings #105520

Uh oh!

Conversation

fschlimb commented Aug 21, 2024

Uh oh!

llvmbot commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kuhar Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fschlimb Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuhar Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joker-eph Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fschlimb Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MacDue left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rengolin Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvmbot commented Aug 21, 2024 •

edited

Loading

kuhar Aug 21, 2024 •

edited

Loading

fschlimb Aug 21, 2024 •

edited

Loading

kuhar Aug 21, 2024 •

edited

Loading

joker-eph Aug 21, 2024 •

edited

Loading

fschlimb Aug 23, 2024 •

edited

Loading

MacDue left a comment •

edited

Loading

rengolin Aug 22, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading