PNaCl bitcode: Strip pointer types from intrinsic declarations' parameters

Change the writer to strip pointer types from intrinsics' argument and
return types, replacing them with i32.  This simplifies the PNaCl
bitcode format so that pointer types don't need to be represented
here.

Change the reader to restore the pointer types so that the intrinsic
declarations pass the LLVM and PNaCl verifiers.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3671
TEST=intrinsic tests in call-elide.ll
  + intrinsic-pointer-args.ll
  + run small_tests with bitcode v2 enabled

Review URL: https://codereview.chromium.org/23793005

6 files changed, 154 insertions, 1 deletions

diff --git a/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp b/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
index 66cc66b75f..7e9066d300 100644
--- a/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
+++ b/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
@@ -174,6 +174,11 @@ void PNaClABIVerifyModule::checkGlobalValueCommon(const GlobalValue *GV) {
 }
 
 AllowedIntrinsics::AllowedIntrinsics(LLVMContext *Context) : Context(Context) {
+  // Note that new intrinsics added here may also need to be added to
+  // NaClBitcodeReader.cpp if they contain pointer-typed parameters.
+  // TODO(mseaborn): Change NaClBitcodeReader.cpp to reuse the list
+  // below.
+
   Type *I8Ptr = Type::getInt8PtrTy(*Context);
   Type *I8 = Type::getInt8Ty(*Context);
   Type *I16 = Type::getInt16Ty(*Context);
diff --git a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
index 937012feb8..8b8e9072a8 100644
--- a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
+++ b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
@@ -177,6 +177,10 @@ void NaClBitcodeReaderValueList::AssignGlobalVar(GlobalVariable *GV,
   ValuePtrs[Idx] = GV;
 }
 
+void NaClBitcodeReaderValueList::OverwriteValue(Value *V, unsigned Idx) {
+  ValuePtrs[Idx] = V;
+}
+
 Value *NaClBitcodeReaderValueList::getValueFwdRef(unsigned Idx) {
   if (Idx >= size())
     return 0;
@@ -771,6 +775,60 @@ bool NaClBitcodeReader::GlobalCleanup() {
   return false;
 }
 
+FunctionType *NaClBitcodeReader::AddPointerTypesToIntrinsicType(
+    StringRef Name, FunctionType *FTy) {
+  Type *ReturnTy = FTy->getReturnType();
+  SmallVector<Type *, 8> ArgTypes(FTy->param_begin(), FTy->param_end());
+
+  // Ideally we wouldn't need a list of supported intrinsics here, but
+  // Intrinsic::* doesn't provide a function for recovering the
+  // expected type of an intrinsic given its full name.
+  // TODO(mseaborn): We could reuse the intrinsic list from
+  // PNaClABIVerifyModule.cpp here.
+  if (Name == "llvm.nacl.read.tp" ||
+      Name == "llvm.stacksave") {
+    ReturnTy = Type::getInt8PtrTy(Context);
+  } else if (Name == "llvm.nacl.setjmp" ||
+             Name == "llvm.nacl.longjmp" ||
+             Name == "llvm.stackrestore" ||
+             Name.startswith("llvm.memset.")) {
+    assert(ArgTypes.size() >= 1);
+    ArgTypes[0] = Type::getInt8PtrTy(Context);
+  } else if (Name.startswith("llvm.memcpy.") ||
+             Name.startswith("llvm.memmove.")) {
+    assert(ArgTypes.size() >= 2);
+    ArgTypes[0] = Type::getInt8PtrTy(Context);
+    ArgTypes[1] = Type::getInt8PtrTy(Context);
+  } else if (Name.startswith("llvm.nacl.atomic.load.") ||
+             Name.startswith("llvm.nacl.atomic.cmpxchg.")) {
+    assert(ArgTypes.size() >= 1);
+    ArgTypes[0] = ReturnTy->getPointerTo();
+  } else if (Name.startswith("llvm.nacl.atomic.store.")) {
+    assert(ArgTypes.size() >= 2);
+    ArgTypes[1] = ArgTypes[0]->getPointerTo();
+  } else if (Name.startswith("llvm.nacl.atomic.rmw.")) {
+    assert(ArgTypes.size() >= 3);
+    ArgTypes[1] = ArgTypes[2]->getPointerTo();
+  }
+  return FunctionType::get(ReturnTy, ArgTypes, false);
+}
+
+void NaClBitcodeReader::AddPointerTypesToIntrinsicParams() {
+  for (unsigned Index = 0, E = ValueList.size(); Index < E; ++Index) {
+    if (Function *Func = dyn_cast<Function>(ValueList[Index])) {
+      if (Func->isIntrinsic()) {
+        FunctionType *FTy = AddPointerTypesToIntrinsicType(
+            Func->getName(), Func->getFunctionType());
+        Function *NewIntrinsic = Function::Create(
+            FTy, GlobalValue::ExternalLinkage, "", TheModule);
+        NewIntrinsic->takeName(Func);
+        ValueList.OverwriteValue(NewIntrinsic, Index);
+        Func->eraseFromParent();
+      }
+    }
+  }
+}
+
 bool NaClBitcodeReader::ParseModule(bool Resume) {
   DEBUG(dbgs() << "-> ParseModule\n");
   if (Resume)
@@ -816,6 +874,11 @@ bool NaClBitcodeReader::ParseModule(bool Resume) {
         if (ParseValueSymbolTable())
           return true;
         SeenValueSymbolTable = true;
+        if (GetPNaClVersion() >= 2) {
+          // Now that we know the names of the intrinsics, we can add
+          // pointer types to the intrinsic declarations' types.
+          AddPointerTypesToIntrinsicParams();
+        }
         break;
       case naclbitc::FUNCTION_BLOCK_ID:
         // If this is the first function body we've seen, reverse the
diff --git a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h
index 318fcb1470..56a894c7d4 100644
--- a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h
+++ b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h
@@ -130,6 +130,10 @@ public:
   // replaces uses of the global variable forward reference with the
   // value GV.
   void AssignGlobalVar(GlobalVariable *GV, unsigned Idx);
+
+  // Assigns Idx to the given value, overwriting the existing entry
+  // and possibly modifying the type of the entry.
+  void OverwriteValue(Value *V, unsigned Idx);
 };
 
 
@@ -333,6 +337,9 @@ private:
   /// \brief Install instruction I into basic block BB.
   bool InstallInstruction(BasicBlock *BB, Instruction *I);
 
+  FunctionType *AddPointerTypesToIntrinsicType(StringRef Name,
+                                               FunctionType *FTy);
+  void AddPointerTypesToIntrinsicParams();
   bool ParseModule(bool Resume);
   bool ParseTypeTable();
   bool ParseTypeTableBody();
diff --git a/lib/Bitcode/NaCl/Writer/NaClValueEnumerator.cpp b/lib/Bitcode/NaCl/Writer/NaClValueEnumerator.cpp
index 64d7661415..42f78baa2a 100644
--- a/lib/Bitcode/NaCl/Writer/NaClValueEnumerator.cpp
+++ b/lib/Bitcode/NaCl/Writer/NaClValueEnumerator.cpp
@@ -298,7 +298,27 @@ void NaClValueEnumerator::EnumerateValue(const Value *VIn) {
 }
 
 
+Type *NaClValueEnumerator::NormalizeParamType(Type *Ty) const {
+  // Strip pointer types.
+  if (Ty->isPointerTy() && PNaClVersion >= 2)
+    Ty = IntPtrType;
+  return Ty;
+}
+
+Type *NaClValueEnumerator::NormalizeType(Type *Ty) const {
+  if (FunctionType *FTy = dyn_cast<FunctionType>(Ty)) {
+    SmallVector<Type *, 8> ArgTypes;
+    for (unsigned I = 0, E = FTy->getNumParams(); I < E; ++I)
+      ArgTypes.push_back(NormalizeParamType(FTy->getParamType(I)));
+    Ty = FunctionType::get(NormalizeParamType(FTy->getReturnType()),
+                           ArgTypes, false);
+  }
+  return Ty;
+}
+
 void NaClValueEnumerator::EnumerateType(Type *Ty, bool InsideOptimizeTypes) {
+  Ty = NormalizeType(Ty);
+
   // The label type does not need to be given a type ID.
   if (Ty->isLabelTy())
     return;
diff --git a/lib/Bitcode/NaCl/Writer/NaClValueEnumerator.h b/lib/Bitcode/NaCl/Writer/NaClValueEnumerator.h
index 71638dd4eb..5f25039e30 100644
--- a/lib/Bitcode/NaCl/Writer/NaClValueEnumerator.h
+++ b/lib/Bitcode/NaCl/Writer/NaClValueEnumerator.h
@@ -106,7 +106,7 @@ public:
   unsigned getValueID(const Value *V) const;
 
   unsigned getTypeID(Type *T) const {
-    TypeMapType::const_iterator I = TypeMap.find(T);
+    TypeMapType::const_iterator I = TypeMap.find(NormalizeType(T));
     assert(I != TypeMap.end() && "Type not in NaClValueEnumerator!");
     return I->second-1;
   }
@@ -158,6 +158,9 @@ private:
   void OptimizeTypes(const Module *M);
   void OptimizeConstants(unsigned CstStart, unsigned CstEnd);
 
+  Type *NormalizeParamType(Type *Ty) const;
+  Type *NormalizeType(Type *Ty) const;
+
   void EnumerateValue(const Value *V);
   void EnumerateType(Type *T, bool InsideOptimizeTypes=false);
   void EnumerateOperandType(const Value *V);
diff --git a/test/NaCl/Bitcode/intrinsic-pointer-args.ll b/test/NaCl/Bitcode/intrinsic-pointer-args.ll
new file mode 100644
index 0000000000..0848568fd3
--- /dev/null
+++ b/test/NaCl/Bitcode/intrinsic-pointer-args.ll
@@ -0,0 +1,55 @@
+; Test that intrinsic declarations are read back correctly.
+
+; RUN: llvm-as < %s | pnacl-freeze --pnacl-version=1 | pnacl-thaw \
+; RUN:              | llvm-dis - | FileCheck %s -check-prefix=TD
+
+; RUN: llvm-as < %s | pnacl-freeze --pnacl-version=2 | pnacl-thaw \
+; RUN:              | llvm-dis - | FileCheck %s -check-prefix=TD
+
+
+declare i8* @llvm.stacksave()
+declare void @llvm.stackrestore(i8*)
+
+declare i8* @llvm.nacl.read.tp()
+declare void @llvm.nacl.longjmp(i8*, i32)
+declare void @llvm.nacl.setjmp(i8*)
+
+declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture, i8* nocapture, i32, i32, i1)
+declare void @llvm.memmove.p0i8.p0i8.i32(i8* nocapture, i8* nocapture, i32, i32, i1)
+declare void @llvm.memset.p0i8.i32(i8* nocapture, i8, i32, i32, i1)
+
+declare i32 @llvm.nacl.atomic.load.i32(i32*, i32)
+declare i64 @llvm.nacl.atomic.load.i64(i64*, i32)
+
+declare i32 @llvm.nacl.atomic.cmpxchg.i32(i32*, i32, i32, i32, i32)
+declare i64 @llvm.nacl.atomic.cmpxchg.i64(i64*, i64, i64, i32, i32)
+
+declare void @llvm.nacl.atomic.store.i32(i32, i32*, i32)
+declare void @llvm.nacl.atomic.store.i64(i64, i64*, i32)
+
+declare i32 @llvm.nacl.atomic.rmw.i32(i32, i32*, i32, i32)
+declare i64 @llvm.nacl.atomic.rmw.i64(i32, i64*, i64, i32)
+
+
+; TD: declare i8* @llvm.stacksave()
+; TD: declare void @llvm.stackrestore(i8*)
+
+; TD: declare i8* @llvm.nacl.read.tp()
+; TD: declare void @llvm.nacl.longjmp(i8*, i32)
+; TD: declare void @llvm.nacl.setjmp(i8*)
+
+; TD: declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture, i8* nocapture, i32, i32, i1)
+; TD: declare void @llvm.memmove.p0i8.p0i8.i32(i8* nocapture, i8* nocapture, i32, i32, i1)
+; TD: declare void @llvm.memset.p0i8.i32(i8* nocapture, i8, i32, i32, i1)
+
+; TD: declare i32 @llvm.nacl.atomic.load.i32(i32*, i32)
+; TD: declare i64 @llvm.nacl.atomic.load.i64(i64*, i32)
+
+; TD: declare i32 @llvm.nacl.atomic.cmpxchg.i32(i32*, i32, i32, i32, i32)
+; TD: declare i64 @llvm.nacl.atomic.cmpxchg.i64(i64*, i64, i64, i32, i32)
+
+; TD: declare void @llvm.nacl.atomic.store.i32(i32, i32*, i32)
+; TD: declare void @llvm.nacl.atomic.store.i64(i64, i64*, i32)
+
+; TD: declare i32 @llvm.nacl.atomic.rmw.i32(i32, i32*, i32, i32)
+; TD: declare i64 @llvm.nacl.atomic.rmw.i64(i32, i64*, i64, i32)