[IR] Remove temporary const operator created in Value::getPointerAlignment()

[mlychkov]

Summary

New temporary constant operator may be created in getPointerAlignment() function.
This instance is not used by any instruction. However later it may lead to errors during
code processing as not expected.
Remove this constant after alignment calculation as no more required.

[lebedev.ri]

I don't believe the question

In D73131#1865791, @lebedev.ri wrote:

In D73131#1861998, @jdoerfert wrote:

In D73131#1861978, @lebedev.ri wrote:

In D73131#1861781, @jdoerfert wrote:

In D73131#1861624, @mlychkov wrote:

In D73131#1861595, @mlychkov wrote:

...
Is this alignment accounting intended to be used for getelementptr instructions too?
If yes then may we cleanup extra user after we get TrailingZeros value?

Or is there a way to perform this computation without const_cast?

The problem is ConstantExpr::getPtrToInt. Do we striclty need that?

That is the workhorse here, we can't get around using it.

Without seeing the code where this is causing trouble i'm somewhat unsure
on how this is a problem, or how to test that the fix would actually fix it.
Perhaps, after computing TrailingZeros, we need to explicitly DCE CstInt?

That'll probably work as well. If CstInt has 0 users it can be removed.

Do we have an established precedent on this kind of issues?
I don't recall ever seeing such a problem before,
nor do i recall seeing manual cleaning up of tmp constantexprs.
I'm mainly wondering whether we actually have such guarantees that this violated (should not create new users)?

In D73131#1865811, @nikic wrote:

Sounds pretty odd to me too. From a quick grep, there's only a handful of destroyConstant() uses outside GlobalOpt, and those tend to deal with BlockAddress only (which is somewhat particular about uses).

was answered. Do we have this guarantee in the first place?

[mlychkov]

In D74700#1878635, @lebedev.ri wrote:

I don't believe the question

In D73131#1865811, @nikic wrote:

Sounds pretty odd to me too. From a quick grep, there's only a handful of destroyConstant() uses outside GlobalOpt, and those tend to deal with BlockAddress only (which is somewhat particular about uses).

was answered. Do we have this guarantee in the first place?

Maybe, we don't have it. But I thought it wasn't a good practice to change something in get-methods (except caches, maybe). If we can cleanup instances after some unexpected changes why not to do so?

[efriedma]

If we can cleanup instances after some unexpected changes why not to do so?

The big problem here is that getPointerAlignment could be used in places where not all uses of a constant are actual "Use" instances. Then you have a really subtle use-after-free. (This could happen in places with maps on the side, like frontends or complex transforms).

If you really want to avoid creating extra constants, probably the right strategy is to avoid calling getPtrToInt in the first place. There aren't that many ways to construct a pointer that getPtrToInt can fold to a ConstantInt, anyway. (Off the top of my head, it would have to be null, an inttoptr, or a gep of one of those.)

[jdoerfert]

In D74700#1879625, @efriedma wrote:

If we can cleanup instances after some unexpected changes why not to do so?

The big problem here is that getPointerAlignment could be used in places where not all uses of a constant are actual "Use" instances. Then you have a really subtle use-after-free. (This could happen in places with maps on the side, like frontends or complex transforms).

If you really want to avoid creating extra constants, probably the right strategy is to avoid calling getPtrToInt in the first place. There aren't that many ways to construct a pointer that getPtrToInt can fold to a ConstantInt, anyway. (Off the top of my head, it would have to be null, an inttoptr, or a gep of one of those.)

I was hoping that would be a viable solution. Maybe matching null explicitly and otherwise just dealing with the constant that we have. IIRC, @lebedev.ri mentioned that would not work well but I let him explain.

[lebedev.ri]

In D74700#1879865, @jdoerfert wrote:

In D74700#1879625, @efriedma wrote:

If we can cleanup instances after some unexpected changes why not to do so?

The big problem here is that getPointerAlignment could be used in places where not all uses of a constant are actual "Use" instances. Then you have a really subtle use-after-free. (This could happen in places with maps on the side, like frontends or complex transforms).

If you really want to avoid creating extra constants, probably the right strategy is to avoid calling getPtrToInt in the first place. There aren't that many ways to construct a pointer that getPtrToInt can fold to a ConstantInt, anyway. (Off the top of my head, it would have to be null, an inttoptr, or a gep of one of those.)

I was hoping that would be a viable solution. Maybe matching null explicitly and otherwise just dealing with the constant that we have. IIRC, @lebedev.ri mentioned that would not work well but I let him explain.

I mean, sure, we can avoid using getPtrToInt and hardcode what it does here, but still, why?
Either the current state breaks guarantees and we should fix it,
or it does not and this roughly falls under the "c++ api is unstable" and whatever code
was relying on the unwritten guarantee should be relaxed not to rely on it.

[mlychkov]

When reading the following definition of method 'MaybeAlign Value::getPointerAlignment(const DataLayout &DL) const' most users assume that Value won't be modified by this method. If we don't guarantee it then it would be nice to remove const-qualifier and change name on something like 'accountPointerAlignment'. As renaming is less possible then a comment might be present that this method may change an IR.

[efriedma]

The general rule for IR passes is that dangling constants may exist, and transformations are expected to correctly handle them (ignore or erase them, as appropriate). So creating them should be harmless across IR passes.

That's not really an excuse for IR queries that don't obviously involve creating constants to do so, though: it could be confusing within a pass. For example, if a pass is trying to iterate over a use list, and asking for the alignment modifies that use list.

[mlychkov]

I'm closing this review as current patch is not fixing the issue properly.

llvm/lib/IR/Value.cpp

if (!Alignment && Call->getCalledFunction())
           Call->getCalledFunction()->getAttributes().getRetAlignment());
     return Alignment;
   } else if (const LoadInst *LI = dyn_cast<LoadInst>(this)) {
     if (MDNode *MD = LI->getMetadata(LLVMContext::MD_align)) {
       ConstantInt *CI = mdconst::extract<ConstantInt>(MD->getOperand(0));
       return MaybeAlign(CI->getLimitedValue());
     }
   } else if (auto *CstPtr = dyn_cast<Constant>(this)) {
-    if (auto *CstInt = dyn_cast_or_null<ConstantInt>(ConstantExpr::getPtrToInt(
-            const_cast<Constant *>(CstPtr), DL.getIntPtrType(getType()),
-            /*OnlyIfReduced=*/true))) {
+    MaybeAlign Alignment(llvm::None);
+    // NOTE: getPtrToInt call may create extra constant operator
+    Constant *CstPTI = ConstantExpr::getPtrToInt(const_cast<Constant *>(CstPtr),
+                                                 DL.getIntPtrType(getType()),
+                                                 /*OnlyIfReduced=*/true);
+    if (auto *CstInt = dyn_cast_or_null<ConstantInt>(CstPTI)) {
       size_t TrailingZeros = CstInt->getValue().countTrailingZeros();
       // While the actual alignment may be large, elsewhere we have
-      // an arbitrary upper alignmet limit, so let's clamp to it.
-      return Align(TrailingZeros < Value::MaxAlignmentExponent
+      // an arbitrary upper alignment limit, so let's clamp to it.
+      Alignment = Align(TrailingZeros < Value::MaxAlignmentExponent
                             ? uint64_t(1) << TrailingZeros
                             : Value::MaximumAlignment);
     }
+    // Remove unused ptrtoint operator if it has been created
+    if (CstPTI && !CstPTI->isConstantUsed())
+      CstPTI->destroyConstant();
+    return Alignment;
   }
   return llvm::None;
 }
 
 const Value *Value::DoPHITranslation(const BasicBlock *CurBB,
                                      const BasicBlock *PredBB) const {
   auto *PN = dyn_cast<PHINode>(this);
   if (PN && PN->getParent() == CurBB)

llvm/unittests/IR/ConstantsTest.cpp

TEST(ConstantsTest, isElementWiseEqual) {
   Constant *CP00U = ConstantVector::get({CP0, CP0, CPU});
 
   EXPECT_FALSE(CP0000->isElementWiseEqual(CP00U0));
   EXPECT_FALSE(CP00U0->isElementWiseEqual(CP0000));
   EXPECT_FALSE(CP0000->isElementWiseEqual(CP00U));
   EXPECT_FALSE(CP00U->isElementWiseEqual(CP00U0));
 }
 
+TEST(ConstantsTest, GlobalValueImmutabilityOnGetPtrAlignmentTest) {
+  LLVMContext Context;
+  SMDiagnostic Error;
+  std::unique_ptr<Module> M = parseAssemblyString(
+      "@gs = common global [8 x i32] zeroinitializer, align 8 "
+      "define void @f() { "
+      "entry: "
+      "  %0 = load i32, i32* getelementptr inbounds "
+      "         ([8 x i32], [8 x i32]* @gs, i64 0, i64 0), align 8 "
+      "  ret void "
+      "}",
+      Error, Context);
+
+  const Function *F = M->getFunction("f");
+  const BasicBlock &BB = F->getEntryBlock();
+  const Instruction &I = BB.front();
+  const Value *GEP = I.getOperand(0);
+  EXPECT_TRUE(isa<Constant>(GEP));
+
+  GlobalVariable *GV = M->getGlobalVariable("gs");
+  unsigned GVUsesNumBefore = GV->getNumUses();
+
+  MaybeAlign Align = GEP->getPointerAlignment(DataLayout(""));
+  (void)Align;
+
+  ASSERT_EQ(GVUsesNumBefore, GV->getNumUses());
+}
+
 }  // end anonymous namespace
 }  // end namespace llvm