Address review feedback.

- LangRef.rst: missing ':', stale example
- LLParser.cpp: auto *
- Bitcode: remove ptrty/addrdiscty
- AsmWriter.cpp: ListSeparator
- Constants.h: cast<>
- Constants.cpp: stale NDEBUG
- Constants.cpp: stale comments
- Constants.cpp: use getTypeSizeInBits for offset accumulation,
  to allow pointer/integer types without fiddling.
- ConstantFold.cpp: remove FIXME, should be addressed separately
- ValueTracking.cpp: remove from isGuaranteedNotToBeUndefOrPoison

Author: ahmedbougacha

llvm/docs/LangRef.rst

@@ -4750,7 +4750,7 @@ reference to the CFI jump table in the ``LowerTypeTests`` pass. These constants
 may be useful in low-level programs, such as operating system kernels, which
 need to refer to the actual function body.
 
-.. _ptrauth
+.. _ptrauth:
 
 Authenticated Pointers
 ----------------------
@@ -4775,8 +4775,6 @@ If the address discriminator is present, then it is
     %tmp2 = call i64 @llvm.ptrauth.sign.i64(i64 ptrtoint (ptr CST to i64), i64  %tmp1)
     %val = inttoptr i64 %tmp2 to ptr
 
-    %tmp = call i64 @llvm.ptrauth.blend.i64
-
 .. _constantexprs:
 
 Constant Expressions

llvm/include/llvm/Bitcode/LLVMBitCodes.h

@@ -412,9 +412,7 @@ enum ConstantsCodes {
                                       //                 asmdialect|unwind,
                                       //                 asmstr,conststr]
   CST_CODE_CE_GEP_WITH_INRANGE = 31,  // [opty, flags, range, n x operands]
-  CST_CODE_SIGNED_PTR = 32,           // SIGNED_PTR: [ptrty, ptr, key,
-                                      //              addrdiscty, addrdisc,
-                                      //              disc]
+  CST_CODE_SIGNED_PTR = 32,           // [ptr, key, addrdisc, disc]
 };
 
 /// CastOpcodes - These are values used in the bitcode files to encode which

llvm/include/llvm/IR/Constants.h

@@ -1034,18 +1034,22 @@ class ConstantPtrAuth final : public Constant {
   DECLARE_TRANSPARENT_OPERAND_ACCESSORS(Constant);
 
   /// The pointer that is authenticated in this authenticated global reference.
-  Constant *getPointer() const { return (Constant *)Op<0>().get(); }
+  Constant *getPointer() const { return cast<Constant>(Op<0>().get()); }
 
   /// The Key ID, an i32 constant.
-  ConstantInt *getKey() const { return (ConstantInt *)Op<1>().get(); }
+  ConstantInt *getKey() const { return cast<ConstantInt>(Op<1>().get()); }
 
   /// The address discriminator if any, or the null constant.
   /// If present, this must be a value equivalent to the storage location of
   /// the only user of the authenticated ptrauth global.
-  Constant *getAddrDiscriminator() const { return (Constant *)Op<2>().get(); }
+  Constant *getAddrDiscriminator() const {
+    return cast<Constant>(Op<2>().get());
+  }
 
   /// The discriminator.
-  ConstantInt *getDiscriminator() const { return (ConstantInt *)Op<3>().get(); }
+  ConstantInt *getDiscriminator() const {
+    return cast<ConstantInt>(Op<3>().get());
+  }
 
   /// Whether there is any non-null address discriminator.
   bool hasAddressDiversity() const {

llvm/lib/Analysis/ValueTracking.cpp

@@ -7226,9 +7226,6 @@ static bool isGuaranteedNotToBeUndefOrPoison(
         isa<ConstantPointerNull>(C) || isa<Function>(C))
       return true;
 
-    if (isa<ConstantPtrAuth>(C))
-      return true;
-
     if (C->getType()->isVectorTy() && !isa<ConstantExpr>(C))
       return (!includesUndef(Kind) ? !C->containsPoisonElement()
                                    : !C->containsUndefOrPoisonElement()) &&

llvm/lib/AsmParser/LLParser.cpp

@@ -4069,15 +4069,15 @@ bool LLParser::parseValID(ValID &ID, PerFunctionState *PFS, Type *ExpectedTy) {
     if (!Ptr->getType()->isPointerTy())
       return error(ID.Loc, "signed pointer must be a pointer");
 
-    auto KeyC = dyn_cast<ConstantInt>(Key);
+    auto *KeyC = dyn_cast<ConstantInt>(Key);
     if (!KeyC || KeyC->getBitWidth() != 32)
       return error(ID.Loc, "signed pointer key must be i32 constant integer");
 
     if (!AddrDisc->getType()->isPointerTy())
       return error(ID.Loc,
                    "signed pointer address discriminator must be a pointer");
 
-    auto DiscC = dyn_cast<ConstantInt>(Disc);
+    auto *DiscC = dyn_cast<ConstantInt>(Disc);
     if (!DiscC || DiscC->getBitWidth() != 64)
       return error(ID.Loc,
                    "signed pointer discriminator must be i64 constant integer");

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

@@ -3643,18 +3643,13 @@ Error BitcodeReader::parseConstants() {
       break;
     }
     case bitc::CST_CODE_SIGNED_PTR: {
-      if (Record.size() < 6)
-        return error("Invalid record");
-      Type *PtrTy = getTypeByID(Record[0]);
-      if (!PtrTy)
-        return error("Invalid record");
-
-      // PtrTy, Ptr, Key, AddrDiscTy, AddrDisc, Disc
+      if (Record.size() < 4)
+        return error("Invalid ptrauth record");
+      // Ptr, Key, AddrDisc, Disc
       V = BitcodeConstant::create(
         Alloc, CurTy, BitcodeConstant::ConstantPtrAuthOpcode,
-        {(unsigned)Record[1], (unsigned)Record[2], (unsigned)Record[4],
-         (unsigned)Record[5]});
-
+        {(unsigned)Record[0], (unsigned)Record[1], (unsigned)Record[2],
+         (unsigned)Record[3]});
       break;
     }
     }

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

@@ -2822,14 +2822,6 @@ void ModuleBitcodeWriter::writeConstants(unsigned FirstVal, unsigned LastVal,
       Record.push_back(VE.getTypeID(BA->getFunction()->getType()));
       Record.push_back(VE.getValueID(BA->getFunction()));
       Record.push_back(VE.getGlobalBasicBlockID(BA->getBasicBlock()));
-    } else if (const ConstantPtrAuth *SP = dyn_cast<ConstantPtrAuth>(C)) {
-      Code = bitc::CST_CODE_SIGNED_PTR;
-      Record.push_back(VE.getTypeID(SP->getPointer()->getType()));
-      Record.push_back(VE.getValueID(SP->getPointer()));
-      Record.push_back(VE.getValueID(SP->getKey()));
-      Record.push_back(VE.getTypeID(SP->getAddrDiscriminator()->getType()));
-      Record.push_back(VE.getValueID(SP->getAddrDiscriminator()));
-      Record.push_back(VE.getValueID(SP->getDiscriminator()));
     } else if (const auto *Equiv = dyn_cast<DSOLocalEquivalent>(C)) {
       Code = bitc::CST_CODE_DSO_LOCAL_EQUIVALENT;
       Record.push_back(VE.getTypeID(Equiv->getGlobalValue()->getType()));
@@ -2838,6 +2830,12 @@ void ModuleBitcodeWriter::writeConstants(unsigned FirstVal, unsigned LastVal,
       Code = bitc::CST_CODE_NO_CFI_VALUE;
       Record.push_back(VE.getTypeID(NC->getGlobalValue()->getType()));
       Record.push_back(VE.getValueID(NC->getGlobalValue()));
+    } else if (const auto *CPA = dyn_cast<ConstantPtrAuth>(C)) {
+      Code = bitc::CST_CODE_SIGNED_PTR;
+      Record.push_back(VE.getValueID(CPA->getPointer()));
+      Record.push_back(VE.getValueID(CPA->getKey()));
+      Record.push_back(VE.getValueID(CPA->getAddrDiscriminator()));
+      Record.push_back(VE.getValueID(CPA->getDiscriminator()));
     } else {
 #ifndef NDEBUG
       C->dump();

llvm/lib/IR/AsmWriter.cpp

@@ -1592,15 +1592,13 @@ static void WriteConstantInternal(raw_ostream &Out, const Constant *CV,
 
   if (const ConstantPtrAuth *SP = dyn_cast<ConstantPtrAuth>(CV)) {
     Out << "ptrauth (";
-
+    ListSeparator LS;
     for (unsigned i = 0; i < SP->getNumOperands(); ++i) {
+      Out << LS;
       WriterCtx.TypePrinter->print(SP->getOperand(i)->getType(), Out);
       Out << ' ';
       WriteAsOperandInternal(Out, SP->getOperand(i), WriterCtx);
-      if (i != SP->getNumOperands() - 1)
-        Out << ", ";
     }
-
     Out << ')';
     return;
   }

llvm/lib/IR/ConstantFold.cpp

@@ -1154,9 +1154,6 @@ static ICmpInst::Predicate evaluateICmpRelation(Constant *V1, Constant *V2) {
                                 GV->getType()->getAddressSpace()))
         return ICmpInst::ICMP_UGT;
     }
-  } else if (const ConstantPtrAuth *SP = dyn_cast<ConstantPtrAuth>(V1)) {
-    // FIXME: ahmedbougacha: implement ptrauth cst comparison
-    return ICmpInst::BAD_ICMP_PREDICATE;
   } else if (auto *CE1 = dyn_cast<ConstantExpr>(V1)) {
     // Ok, the LHS is known to be a constantexpr.  The RHS can be any of a
     // constantexpr, a global, block address, or a simple constant.

llvm/lib/IR/Constants.cpp

@@ -2023,18 +2023,18 @@ Value *NoCFIValue::handleOperandChangeImpl(Value *From, Value *To) {
 
 static bool areEquivalentAddrDiscriminators(const Value *V1, const Value *V2,
                                             const DataLayout &DL) {
-  APInt V1Off(DL.getPointerSizeInBits(), 0);
-  APInt V2Off(DL.getPointerSizeInBits(), 0);
+  APInt Off1(DL.getTypeSizeInBits(V1->getType()), 0);
+  APInt Off2(DL.getTypeSizeInBits(V2->getType()), 0);
 
   if (auto *V1Cast = dyn_cast<PtrToIntOperator>(V1))
     V1 = V1Cast->getPointerOperand();
   if (auto *V2Cast = dyn_cast<PtrToIntOperator>(V2))
     V2 = V2Cast->getPointerOperand();
   auto *V1Base = V1->stripAndAccumulateConstantOffsets(
-      DL, V1Off, /*AllowNonInbounds=*/true);
+      DL, Off1, /*AllowNonInbounds=*/true);
   auto *V2Base = V2->stripAndAccumulateConstantOffsets(
-      DL, V2Off, /*AllowNonInbounds=*/true);
-  return V1Base == V2Base && V1Off == V2Off;
+      DL, Off2, /*AllowNonInbounds=*/true);
+  return V1Base == V2Base && Off1 == Off2;
 }
 
 bool ConstantPtrAuth::isCompatibleWith(const Value *Key,
@@ -2085,12 +2085,10 @@ ConstantPtrAuth *ConstantPtrAuth::get(Constant *Ptr, ConstantInt *Key,
 ConstantPtrAuth::ConstantPtrAuth(Constant *Ptr, ConstantInt *Key,
                                  Constant *AddrDisc, ConstantInt *Disc)
     : Constant(Ptr->getType(), Value::ConstantPtrAuthVal, &Op<0>(), 4) {
-#ifndef NDEBUG
   assert(Ptr->getType()->isPointerTy());
   assert(Key->getBitWidth() == 32);
   assert(AddrDisc->getType()->isPointerTy());
   assert(Disc->getBitWidth() == 64);
-#endif
   setOperand(0, Ptr);
   setOperand(1, Key);
   setOperand(2, AddrDisc);
@@ -2106,11 +2104,9 @@ Value *ConstantPtrAuth::handleOperandChangeImpl(Value *From, Value *ToV) {
   assert(isa<Constant>(ToV) && "Cannot make Constant refer to non-constant!");
   Constant *To = cast<Constant>(ToV);
 
-  SmallVector<Constant *, 8> Values;
-  Values.reserve(getNumOperands()); // Build replacement array.
+  SmallVector<Constant *, 4> Values;
+  Values.reserve(getNumOperands());
 
-  // Fill values with the modified operands of the constant array.  Also,
-  // compute whether this turns into an all-zeros array.
   unsigned NumUpdated = 0;
 
   Use *OperandList = getOperandList();
@@ -2125,7 +2121,6 @@ Value *ConstantPtrAuth::handleOperandChangeImpl(Value *From, Value *ToV) {
     Values.push_back(Val);
   }
 
-  // FIXME: shouldn't we check it's not already there?
   return getContext().pImpl->ConstantPtrAuths.replaceOperandsInPlace(
       Values, this, From, To, NumUpdated, OperandNo);
 }