[IR] Drop const in DILocation::getMergedLocation

[Dinistro]

Summary

This commit removes constness from DILocation::getMergedLocation and
fixes all its users accordingly.

Having constness on the parameters forced the return type to be const
as well, which does force usage of const_cast when the location needs
to be used in metadata nodes.

[Dinistro]

I'm not sure who should review this. If anyone has someone to assign, feel free to do so.

[ftynse]

LGTM

[dblaikie]

any particular examples of these problematic const_casts? (is there a subsequent cleanup patch showing the motivation/value in this patch?)

[Dinistro]

In D149942#4351081, @dblaikie wrote:

any particular examples of these problematic const_casts? (is there a subsequent cleanup patch showing the motivation/value in this patch?)

DILocation has a special relationship with constness. This revision removes the traces of constness in MLIR's import, which was the main motivation for this cleanup.

In general, once DILocations are treated like metadata, they are usually stripped from their constness, which can cause UB.
For example, the DebugLoc constructors (https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/DebugLoc.cpp#L17) contain const_casts to strip away constness, which seems dangerous considering that they provided accessors to that casted pointer.

Currently, we have no plans to push this further, as constness changes can be invasive, and we do not have the resources to commit to this.

[dblaikie]

In D149942#4359606, @Dinistro wrote:

In D149942#4351081, @dblaikie wrote:

any particular examples of these problematic const_casts? (is there a subsequent cleanup patch showing the motivation/value in this patch?)

DILocation has a special relationship with constness. This revision removes the traces of constness in MLIR's import, which was the main motivation for this cleanup.

"MLIR's import"? Did this allow the removal of some const_casts in MLIR? (could you link to those patches here - be good to have the context connected here)

[Dinistro]

In D149942#4362991, @dblaikie wrote:

"MLIR's import"? Did this allow the removal of some const_casts in MLIR? (could you link to those patches here - be good to have the context connected here)

I was referring to the LLVM IR import into MLIR. DebugTranslation.cpp, which was changed in this revision is a part of this. The removal of constness not only drops the need for const_cast in this file, but also affect down stream users of the location translation.
For now, there are no other revisions we can link here, as this is the only place where this is used in MLIR.

[dblaikie]

Fair enough. Thanks for the context!

llvm/include/llvm/IR/DebugInfoMetadata.h

public:
   /// * if both match, keep the line and column;
   /// * if only the line number matches, keep the line and set the column as 0;
   /// * otherwise set line and column as 0.
   /// If they do not share a common scope the location is ambiguous and can't be
   /// represented in a line entry. In this case, set line and column as 0 and
   /// use the scope of any location.
   ///
   /// \p LocA \p LocB: The locations to be merged.
-  static const DILocation *getMergedLocation(const DILocation *LocA,
-                                             const DILocation *LocB);
+  static DILocation *getMergedLocation(DILocation *LocA, DILocation *LocB);
 
   /// Try to combine the vector of locations passed as input in a single one.
   /// This function applies getMergedLocation() repeatedly left-to-right.
   ///
   /// \p Locs: The locations to be merged.
-  static const DILocation *
-  getMergedLocations(ArrayRef<const DILocation *> Locs);
+  static DILocation *getMergedLocations(ArrayRef<DILocation *> Locs);
 
   /// Return the masked discriminator value for an input discrimnator value D
   /// (i.e. zero out the (B+1)-th and above bits for D (B is 0-base).
   // Example: an input of (0x1FF, 7) returns 0xFF.
   static unsigned getMaskedDiscriminator(unsigned D, unsigned B) {
     return (D & getN1Bits(B));
   }
 

llvm/include/llvm/IR/Instruction.h

public:
   ///  1) Simplicity: it takes 2 locations instead of a list of locations.
   ///  2) In worst case, it increases the complexity from O(N*I) to
   ///     O(2*N*I), where N is # of Instructions to merge, and I is the
   ///     maximum level of inline stack. So it is still linear.
   ///  3) Merging of call instructions should be extremely rare in real
   ///     applications, thus the N-way merging should be in code path.
   /// The DebugLoc attached to this instruction will be overwritten by the
   /// merged DebugLoc.
-  void applyMergedLocation(const DILocation *LocA, const DILocation *LocB);
+  void applyMergedLocation(DILocation *LocA, DILocation *LocB);
 
   /// Updates the debug location given that the instruction has been hoisted
   /// from a block to a predecessor of that block.
   /// Note: it is undefined behavior to call this on an instruction not
   /// currently inserted into a function.
   void updateLocationAfterHoist();
 
   /// Drop the instruction's debug location. This does not guarantee removal

llvm/lib/IR/DebugInfo.cpp

 
 unsigned llvm::getDebugMetadataVersionFromModule(const Module &M) {
   if (auto *Val = mdconst::dyn_extract_or_null<ConstantInt>(
           M.getModuleFlag("Debug Info Version")))
     return Val->getZExtValue();
   return 0;
 }
 
-void Instruction::applyMergedLocation(const DILocation *LocA,
-                                      const DILocation *LocB) {
+void Instruction::applyMergedLocation(DILocation *LocA, DILocation *LocB) {
   setDebugLoc(DILocation::getMergedLocation(LocA, LocB));
 }
 
 void Instruction::mergeDIAssignID(
     ArrayRef<const Instruction *> SourceInstructions) {
   // Replace all uses (and attachments) of all the DIAssignIDs
   // on SourceInstructions with a single merged value.
   assert(getFunction() && "Uninserted instruction merged");

llvm/lib/IR/DebugInfoMetadata.cpp

DILocation *DILocation::getImpl(LLVMContext &Context, unsigned Line,
   Ops.push_back(Scope);
   if (InlinedAt)
     Ops.push_back(InlinedAt);
   return storeImpl(new (Ops.size(), Storage) DILocation(
                        Context, Storage, Line, Column, Ops, ImplicitCode),
                    Storage, Context.pImpl->DILocations);
 }
 
-const DILocation *
-DILocation::getMergedLocations(ArrayRef<const DILocation *> Locs) {
+DILocation *DILocation::getMergedLocations(ArrayRef<DILocation *> Locs) {
   if (Locs.empty())
     return nullptr;
   if (Locs.size() == 1)
     return Locs[0];
   auto *Merged = Locs[0];
-  for (const DILocation *L : llvm::drop_begin(Locs)) {
+  for (DILocation *L : llvm::drop_begin(Locs)) {
     Merged = getMergedLocation(Merged, L);
     if (Merged == nullptr)
       break;
   }
   return Merged;
 }
 
-const DILocation *DILocation::getMergedLocation(const DILocation *LocA,
-                                                const DILocation *LocB) {
+DILocation *DILocation::getMergedLocation(DILocation *LocA, DILocation *LocB) {
   if (!LocA || !LocB)
     return nullptr;
 
   if (LocA == LocB)
     return LocA;
 
   LLVMContext &C = LocA->getContext();
 

llvm/lib/Transforms/Scalar/LICM.cpp

bool llvm::promoteLoopAccessesToScalars(
 
   ORE->emit([&]() {
     return OptimizationRemark(DEBUG_TYPE, "PromoteLoopAccessesToScalar",
                               LoopUses[0])
            << "Moving accesses to memory location out of the loop";
   });
 
   // Look at all the loop uses, and try to merge their locations.
-  std::vector<const DILocation *> LoopUsesLocs;
+  std::vector<DILocation *> LoopUsesLocs;
   for (auto *U : LoopUses)
     LoopUsesLocs.push_back(U->getDebugLoc().get());
   auto DL = DebugLoc(DILocation::getMergedLocations(LoopUsesLocs));
 
   // We use the SSAUpdater interface to insert phi nodes as required.
   SmallVector<PHINode *, 16> NewPHIs;
   SSAUpdater SSA(&NewPHIs);
   LoopPromoter Promoter(SomePtr, LoopUses, SSA, ExitBlocks, InsertPts,

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp

Instruction *CanonicalTerm;
     CanonicalTerm->insertInto(CanonicalBB, CanonicalBB->end());
     // If the canonical terminator has operands, rewrite it to take PHI's.
     for (auto I : zip(NewOps, CanonicalTerm->operands()))
       std::get<1>(I) = std::get<0>(I);
   }
 
   // Now, go through each block (with the current terminator type)
   // we've recorded, and rewrite it to branch to the new common block.
-  const DILocation *CommonDebugLoc = nullptr;
+  DILocation *CommonDebugLoc = nullptr;
   for (BasicBlock *BB : BBs) {
     auto *Term = BB->getTerminator();
     assert(Term->getOpcode() == CanonicalTerm->getOpcode() &&
            "All blocks to be tail-merged must be the same "
            "(function-terminating) terminator type.");
 
     // Aha, found a new non-canonical function terminator. If it has operands,
     // forward them to the PHI nodes in the canonical block.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

static void MergeCompatibleInvokesImpl(ArrayRef<InvokeInst *> Invokes,
   // so update the PHI nodes accordingly.
   for (BasicBlock *Succ : successors(MergedInvoke))
     AddPredecessorToBlock(Succ, /*NewPred=*/MergedInvoke->getParent(),
                           /*ExistPred=*/Invokes.front()->getParent());
 
   // And finally, replace the original `invoke`s with an unconditional branch
   // to the block with the merged `invoke`. Also, give that merged `invoke`
   // the merged debugloc of all the original `invoke`s.
-  const DILocation *MergedDebugLoc = nullptr;
+  DILocation *MergedDebugLoc = nullptr;
   for (InvokeInst *II : Invokes) {
     // Compute the debug location common to all the original `invoke`s.
     if (!MergedDebugLoc)
       MergedDebugLoc = II->getDebugLoc();
     else
       MergedDebugLoc =
           DILocation::getMergedLocation(MergedDebugLoc, II->getDebugLoc());
 

mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h

public:
   /// Returns the OpenMP IR builder associated with the LLVM IR module being
   /// constructed.
   llvm::OpenMPIRBuilder *getOpenMPBuilder();
 
   /// Returns the LLVM module in which the IR is being constructed.
   llvm::Module *getLLVMModule() { return llvmModule.get(); }
 
   /// Translates the given location.
-  const llvm::DILocation *translateLoc(Location loc, llvm::DILocalScope *scope);
+  llvm::DILocation *translateLoc(Location loc, llvm::DILocalScope *scope);
 
   /// Translates the given LLVM debug info metadata.
   llvm::Metadata *translateDebugInfo(LLVM::DINodeAttr attr);
 
   /// Translates the contents of the given block to LLVM IR using this
   /// translator. The LLVM IR basic block corresponding to the given block is
   /// expected to exist in the mapping of this translator. Uses `builder` to
   /// translate the IR, leaving it at the end of the block. If `ignoreArguments`

mlir/lib/Target/LLVMIR/DebugTranslation.h

 class DebugTranslation {
 public:
   DebugTranslation(Operation *module, llvm::Module &llvmModule);
 
   /// Finalize the translation of debug information.
   void finalize();
 
   /// Translate the given location to an llvm debug location.
-  const llvm::DILocation *translateLoc(Location loc, llvm::DILocalScope *scope);
+  llvm::DILocation *translateLoc(Location loc, llvm::DILocalScope *scope);
 
   /// Translate the debug information for the given function.
   void translate(LLVMFuncOp func, llvm::Function &llvmFunc);
 
   /// Translate the given LLVM debug metadata to LLVM.
   llvm::DINode *translate(DINodeAttr attr);
 
   /// Translate the given derived LLVM debug metadata to LLVM.
   template <typename DIAttrT>
   auto translate(DIAttrT attr) {
     // Infer the LLVM type from the attribute type.
     using LLVMTypeT = std::remove_pointer_t<decltype(translateImpl(attr))>;
     return cast_or_null<LLVMTypeT>(translate(DINodeAttr(attr)));
   }
 
 private:
   /// Translate the given location to an llvm debug location with the given
   /// scope and inlinedAt parameters.
-  const llvm::DILocation *translateLoc(Location loc, llvm::DILocalScope *scope,
-                                       const llvm::DILocation *inlinedAt);
+  llvm::DILocation *translateLoc(Location loc, llvm::DILocalScope *scope,
+                                 llvm::DILocation *inlinedAt);
 
   /// Create an llvm debug file for the given file path.
   llvm::DIFile *translateFile(StringRef fileName);
 
   /// Translate the given attribute to the corresponding llvm debug metadata.
   llvm::DIType *translateImpl(DINullTypeAttr attr);
   llvm::DIBasicType *translateImpl(DIBasicTypeAttr attr);
   llvm::DICompileUnit *translateImpl(DICompileUnitAttr attr);
private:
 
   /// Constructs a string metadata node from the string attribute. Returns
   /// nullptr if `stringAttr` is null or contains and empty string.
   llvm::MDString *getMDStringOrNull(StringAttr stringAttr);
 
   /// A mapping between mlir location+scope and the corresponding llvm debug
   /// metadata.
   DenseMap<std::tuple<Location, llvm::DILocalScope *, const llvm::DILocation *>,
-           const llvm::DILocation *>
+           llvm::DILocation *>
       locationToLoc;
 
   /// A mapping between debug attribute and the corresponding llvm debug
   /// metadata.
   DenseMap<Attribute, llvm::DINode *> attrToNode;
 
   /// A mapping between filename and llvm debug file.
   /// TODO: Change this to DenseMap<Identifier, ...> when we can

mlir/lib/Target/LLVMIR/DebugTranslation.cpp

llvm::DINode *DebugTranslation::translate(DINodeAttr attr) {
   return node;
 }
 
 //===----------------------------------------------------------------------===//
 // Locations
 //===----------------------------------------------------------------------===//
 
 /// Translate the given location to an llvm debug location.
-const llvm::DILocation *
-DebugTranslation::translateLoc(Location loc, llvm::DILocalScope *scope) {
+llvm::DILocation *DebugTranslation::translateLoc(Location loc,
+                                                 llvm::DILocalScope *scope) {
   if (!debugEmissionIsEnabled)
     return nullptr;
   return translateLoc(loc, scope, /*inlinedAt=*/nullptr);
 }
 
 /// Translate the given location to an llvm DebugLoc.
-const llvm::DILocation *
-DebugTranslation::translateLoc(Location loc, llvm::DILocalScope *scope,
-                               const llvm::DILocation *inlinedAt) {
+llvm::DILocation *DebugTranslation::translateLoc(Location loc,
+                                                 llvm::DILocalScope *scope,
+                                                 llvm::DILocation *inlinedAt) {
   // LLVM doesn't have a representation for unknown.
   if (!scope || isa<UnknownLoc>(loc))
     return nullptr;
 
   // Check for a cached instance.
   auto existingIt = locationToLoc.find(std::make_tuple(loc, scope, inlinedAt));
   if (existingIt != locationToLoc.end())
     return existingIt->second;
 
-  const llvm::DILocation *llvmLoc = nullptr;
+  llvm::DILocation *llvmLoc = nullptr;
   if (auto callLoc = dyn_cast<CallSiteLoc>(loc)) {
     // For callsites, the caller is fed as the inlinedAt for the callee.
-    const auto *callerLoc = translateLoc(callLoc.getCaller(), scope, inlinedAt);
+    auto *callerLoc = translateLoc(callLoc.getCaller(), scope, inlinedAt);
     llvmLoc = translateLoc(callLoc.getCallee(), scope, callerLoc);
 
   } else if (auto fileLoc = dyn_cast<FileLineColLoc>(loc)) {
     llvm::DILocalScope *locationScope = scope;
     // Only construct a new DIFile when no local scope is present. This
     // prioritizes existing DI information when it's present.
     if (!locationScope) {
       auto *file = translateFile(fileLoc.getFilename());

mlir/lib/Target/LLVMIR/LoopAnnotationTranslation.cpp

auto localScopeAttr =
       dyn_cast_or_null<DILocalScopeAttr>(location.getMetadata());
   if (!localScopeAttr)
     return;
   auto *localScope = dyn_cast<llvm::DILocalScope>(
       loopAnnotationTranslation.moduleTranslation.translateDebugInfo(
           localScopeAttr));
   if (!localScope)
     return;
-  const llvm::Metadata *loc =
+  llvm::Metadata *loc =
       loopAnnotationTranslation.moduleTranslation.translateLoc(location,
                                                                localScope);
-  metadataNodes.push_back(const_cast<llvm::Metadata *>(loc));
+  metadataNodes.push_back(loc);
 }
 
 llvm::MDNode *LoopAnnotationConversion::convert() {
   // Reserve operand 0 for loop id self reference.
   auto dummy = llvm::MDNode::getTemporary(ctx, std::nullopt);
   metadataNodes.push_back(dummy.get());
 
   if (FusedLoc startLoc = attr.getStartLoc())

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

llvm::OpenMPIRBuilderConfig Config(
         isDevice, /* IsTargetCodegen */ false,
         /* HasRequiresUnifiedSharedMemory */ false,
         /* OpenMPOffloadMandatory */ false);
     ompBuilder->setConfig(Config);
   }
   return ompBuilder.get();
 }
 
-const llvm::DILocation *
-ModuleTranslation::translateLoc(Location loc, llvm::DILocalScope *scope) {
+llvm::DILocation *ModuleTranslation::translateLoc(Location loc,
+                                                  llvm::DILocalScope *scope) {
   return debugTranslation->translateLoc(loc, scope);
 }
 
 llvm::Metadata *ModuleTranslation::translateDebugInfo(LLVM::DINodeAttr attr) {
   return debugTranslation->translate(attr);
 }
 
 llvm::NamedMDNode *