[OpaquePtr] Introduce option to force all pointers to be opaque pointers

[aeubanks]

Summary

We don't want to start updating tests to use opaque pointers until we're
close to the opaque pointer transition. However, before the transition
we want to run tests as if pointers are opaque pointers to see if there
are any crashes.

At some point when we have a flag to only create opaque pointers in the
bitcode and textual IR readers, and when we have fixed all places that
try to read a pointee type, this flag will be useless. However, until
then, this can help us find issues more easily.

Since the cl::opt is read into LLVMContext, we need to make sure
LLVMContext is created after cl::ParseCommandLineOptions().

Previously ValueEnumerator would visit the value types of global values
via the pointer type, but with opaque pointers we have to manually visit
the value type.

[dblaikie]

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

367–385

Looks like maybe only one of these code changes is tested - perhaps the test should be expanded to have an example of each of these to exercise each fix here? (maybe in separate patches)

llvm/test/Other/force-opaque-ptrs.ll

6–8

Would this still exercise the desired codepath if the function was void() instead of ptr(ptr)? (if I'm understanding correctly, the interesting issue is that the type of f itself becomes an opaque pointer - so, oh, you need /some/ interesting type in the function type that would be missed, yeah? What about void(i32) would that still reveal the issue/validate the fix?)

I ask because it might be more clear which pointer type becoming opaque is the interesting one?

[dexonsmith]

llvm/lib/IR/Type.cpp

688–689

I wonder if this would be better to thread through as a bool set on the LLVMContext during construction:

if (!EltTy->getContext().supportsTypedPointers())
  return get(EltTy->getcontext(), AddressSpace);

Might make it easier to reason about / access elsewhere. (Could still have the command-line option control that...)

llvm/test/Other/force-opaque-ptrs.ll

2

Can you explain why this test uses --force-opaque-pointers? I feel like this test would be more clear if the textual IR actually matched the IR under test (i.e., if it used ptr)... also, then the textual IR won't need to be updated later when typed pointers are removed. But maybe there's some limitation you're working around?

[aeubanks]

only touch functions in ValueEnumerator
move ForceOpaquePointers to LLVMContextImpl (this doesn't currently work in some tools since LLVMContext is created before parsing command line opts)

[aeubanks]

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

367–385

sure, I can start with just functions

llvm/lib/IR/Type.cpp

688–689

I was worried that looking at a cl::opt for every call to PointerType::get() could be noticeable perf wise, adding it to LLVMContext(Impl) would mostly fix that.
However, I tried this approach, turns out in many tools, the LLVMContext is created before parsing command line options. I think this flag makes sense as a global flag, not a per-tool flag that requires calling something like LLVMContext::setForceOpaquePointers().
I could go around and fix all the tools to create an LLVMContext after parsing command line options.

llvm/test/Other/force-opaque-ptrs.ll

2

I assumed that at some point we were going to update all tests from type * to ptr. I think there was a discussion long ago about this but it was never really resolved. We should probably raise this question on llvm-dev.

6–8

this patch is definitely mixing two things, forcing all pointers to be opaque, and the ValueEnumerator stuff

I'm trying to add a test to make sure that we get out a ptr from something that was originally i32*.
I can make @f only have one visible pointer type:

define void @f(i32* %a) {
  ret void
}

would turn into

define void @f(ptr %a) {
  ret void
}

validating this change.

But just testing that doesn't work due to the ValueEnumerator issues introduced by the flag, which is only reproducible with the flag turned on. So if we want to properly test this we need to combine the two patches.

As for what function types cause the crash, even void() causes a crash. But that doesn't show that the flag is working as expected since there's no i32* anywhere.

[dblaikie]

llvm/test/Other/force-opaque-ptrs.ll

2

Hmm, I might just end up adding more confusion to this thread, but I'm not sure how your answer, @aeubanks applies to @dexonsmith's question - perhaps you could rephrase?

My understanding of @dexonsmith's question, combined with what @aeubanks has mentioned elsewhere in the review:

--force-opaque-pointers is needed because this test is intended to test that functionality/flag, including the fix in the bitcode reader for the types of functions.

But it might be that this could be separated - the flag could be added and tested purely with IR reading/writing, showing a pointer type going from typed to opaque through the IR roundtrip?

Then separately a commit that shows the bitcode change/fix?

(not sure that explains all of @dexonsmith's questions)

[nikic]

Assuming there are no problems moving the command line parsing for opt, this LGTM.

Separating testing for the one-line value enumerator change does not seem particularly valuable.

[aeubanks]

move some LLVMContext after command line parsing

[aeubanks]

llvm/test/Other/force-opaque-ptrs.ll

2

I read @dexonsmith's question as "are we actually eventually going to update all llvm tests off of type * into ptr, which my answer is "yes".

Regarding can we separate the --force-opaque-pointers change and the bitcode change, I believe the answer is no. We need some global or function to show the change, and all of those will require some bitcode change because the type of the global/function is now an opaque pointer, so we have to look at the value type of the global/function.

[dexonsmith]

(Apologies for disappearing; I was out for a bit, and then lost track of this review.)

llvm/lib/IR/Type.cpp

688–689

I could go around and fix all the tools to create an LLVMContext after parsing command line options.

Maybe this would be the right thing to do, but I'm fine with not going that far for now if you think it's error-prone.

There were really two suggestions above:

1. Centralize access to the cl::opt via LLVMContext
2. Read the cl::opt only once, during construction of LLVMContext

WDYT of #1 on its own for now? (I.e., use EltTy->getContext().supportsTypedPointers() here, but that just reads the cl::opt.)

(Personally I'm not too worried about perf of this access/call... my intuition is that the branch would be well-predicted, and there's also a DenseMap lookup to hide behind. Of course I could be wrong though.)

llvm/test/Other/force-opaque-ptrs.ll

2

Sorry for any confusion here.

Taking a fresh look, I see how this tests --force-opaque-pointers when reading textual IR, but there's no coverage for reading bitcode. WDYT of this?

; RUN: llvm-as --force-opaque-pointers < %s | llvm-dis | FileCheck %s
; RUN: llvm-as < %s | llvm-dis --force-opaque-pointers | FileCheck %s

llvm/tools/llvm-as/llvm-as.cpp

120

Probably better to leave these moves of LLVMContext out of this patch -- could be committed separately as an NFC prep-commit ("Moving LLVMContext so that cl::opt can control defaults") if you end up reading the cl::opt during construction.

[dexonsmith]

This LGTM, once you add the RUN line for llvm-dis I suggested in the last comment. It's up to you whether to split out the changes for moving the LLVMContext construction after command-line parsing into a prep commit... there could be an argument for keeping it together as well.

llvm/lib/IR/Type.cpp

688–689

Looking again, it looks like you've already done this (#1 and #2)... not sure what I was reading before, thinking you hadn't done this yet :/.

[aeubanks]

add llvm-dis --force-opaque-pointers test

[aeubanks]

It's hard to test the LLVMContext moves without this patch, I'd rather keep it here.

llvm/lib/IR/Type.cpp

688–689

Yeah I originally did #1, and now am doing #2.

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

 }
 
 ValueEnumerator::ValueEnumerator(const Module &M,
                                  bool ShouldPreserveUseListOrder)
     : ShouldPreserveUseListOrder(ShouldPreserveUseListOrder) {
   if (ShouldPreserveUseListOrder)
     UseListOrders = predictUseListOrder(M);
 
   // Enumerate the global variables.
   for (const GlobalVariable &GV : M.globals())
     EnumerateValue(&GV);
 
   // Enumerate the functions.
   for (const Function & F : M) {
     EnumerateValue(&F);
+    EnumerateType(F.getValueType());
     EnumerateAttributes(F.getAttributes());
   }
 
   // Enumerate the aliases.
   for (const GlobalAlias &GA : M.aliases())
     EnumerateValue(&GA);
 
   // Enumerate the ifuncs.
   for (const GlobalIFunc &GIF : M.ifuncs())
     EnumerateValue(&GIF);
 
   // Remember what is the cutoff between globalvalue's and other constants.
   unsigned FirstConstant = Values.size();
 
   // Enumerate the global variable initializers and attributes.
   for (const GlobalVariable &GV : M.globals()) {
     if (GV.hasInitializer())
       EnumerateValue(GV.getInitializer());
     if (GV.hasAttributes())

llvm/lib/IR/LLVMContextImpl.h

#include "llvm/IR/Metadata.def"
   StructTypeSet AnonStructTypes;
   StringMap<StructType*> NamedStructTypes;
   unsigned NamedStructTypesUniqueID = 0;
 
   DenseMap<std::pair<Type *, uint64_t>, ArrayType*> ArrayTypes;
   DenseMap<std::pair<Type *, ElementCount>, VectorType*> VectorTypes;
   // TODO: clean up the following after we no longer support non-opaque pointer
   // types.
+  bool ForceOpaquePointers;
   DenseMap<Type*, PointerType*> PointerTypes;  // Pointers in AddrSpace = 0
   DenseMap<std::pair<Type*, unsigned>, PointerType*> ASPointerTypes;
 
   /// ValueHandles - This map keeps track of all of the value handles that are
   /// watching a Value*.  The Value::HasValueHandle bit is used to know
   /// whether or not a value has an entry in this map.
   using ValueHandlesTy = DenseMap<Value *, ValueHandleBase *>;
   ValueHandlesTy ValueHandles;

llvm/lib/IR/LLVMContextImpl.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "LLVMContextImpl.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/OptBisect.h"
 #include "llvm/IR/Type.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ManagedStatic.h"
 #include <cassert>
 #include <utility>
 
 using namespace llvm;
 
+static cl::opt<bool>
+    ForceOpaquePointersCL("force-opaque-pointers",
+                          cl::desc("Force all pointers to be opaque pointers"),
+                          cl::init(false));
+
 LLVMContextImpl::LLVMContextImpl(LLVMContext &C)
     : DiagHandler(std::make_unique<DiagnosticHandler>()),
-    VoidTy(C, Type::VoidTyID),
-    LabelTy(C, Type::LabelTyID),
-    HalfTy(C, Type::HalfTyID),
-    BFloatTy(C, Type::BFloatTyID),
-    FloatTy(C, Type::FloatTyID),
-    DoubleTy(C, Type::DoubleTyID),
-    MetadataTy(C, Type::MetadataTyID),
-    TokenTy(C, Type::TokenTyID),
-    X86_FP80Ty(C, Type::X86_FP80TyID),
-    FP128Ty(C, Type::FP128TyID),
-    PPC_FP128Ty(C, Type::PPC_FP128TyID),
-    X86_MMXTy(C, Type::X86_MMXTyID),
-    X86_AMXTy(C, Type::X86_AMXTyID),
-    Int1Ty(C, 1),
-    Int8Ty(C, 8),
-    Int16Ty(C, 16),
-    Int32Ty(C, 32),
-    Int64Ty(C, 64),
-    Int128Ty(C, 128) {}
+      VoidTy(C, Type::VoidTyID), LabelTy(C, Type::LabelTyID),
+      HalfTy(C, Type::HalfTyID), BFloatTy(C, Type::BFloatTyID),
+      FloatTy(C, Type::FloatTyID), DoubleTy(C, Type::DoubleTyID),
+      MetadataTy(C, Type::MetadataTyID), TokenTy(C, Type::TokenTyID),
+      X86_FP80Ty(C, Type::X86_FP80TyID), FP128Ty(C, Type::FP128TyID),
+      PPC_FP128Ty(C, Type::PPC_FP128TyID), X86_MMXTy(C, Type::X86_MMXTyID),
+      X86_AMXTy(C, Type::X86_AMXTyID), Int1Ty(C, 1), Int8Ty(C, 8),
+      Int16Ty(C, 16), Int32Ty(C, 32), Int64Ty(C, 64), Int128Ty(C, 128),
+      ForceOpaquePointers(ForceOpaquePointersCL) {}
 
 LLVMContextImpl::~LLVMContextImpl() {
   // NOTE: We need to delete the contents of OwnedModules, but Module's dtor
   // will call LLVMContextImpl::removeModule, thus invalidating iterators into
   // the container. Avoid iterators during this operation:
   while (!OwnedModules.empty())
     delete *OwnedModules.begin();
 

llvm/lib/IR/Type.cpp

 #include "llvm/IR/Constant.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Value.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/MathExtras.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/TypeSize.h"
+#include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <utility>
 
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
 //                         Type Class Implementation
 //===----------------------------------------------------------------------===//
ScalableVectorType *ScalableVectorType::get(Type *ElementType,
   return cast<ScalableVectorType>(Entry);
 }
 
 //===----------------------------------------------------------------------===//
 //                         PointerType Implementation
 //===----------------------------------------------------------------------===//
 
 PointerType *PointerType::get(Type *EltTy, unsigned AddressSpace) {
   assert(EltTy && "Can't get a pointer to <null> type!");
   assert(isValidElementType(EltTy) && "Invalid type for pointer element!");
 
   LLVMContextImpl *CImpl = EltTy->getContext().pImpl;
 
+  if (CImpl->ForceOpaquePointers)
+    return get(EltTy->getContext(), AddressSpace);
+
   // Since AddressSpace #0 is the common case, we special case it.
   PointerType *&Entry = AddressSpace == 0 ? CImpl->PointerTypes[EltTy]
      : CImpl->ASPointerTypes[std::make_pair(EltTy, AddressSpace)];
 
   if (!Entry)
     Entry = new (CImpl->Alloc) PointerType(EltTy, AddressSpace);
   return Entry;
 }

llvm/test/Other/force-opaque-ptrs.ll

+; RUN: llvm-as --force-opaque-pointers < %s | llvm-dis | FileCheck %s
+; RUN: llvm-as < %s | llvm-dis --force-opaque-pointers | FileCheck %s
+; RUN: opt --force-opaque-pointers < %s -S | FileCheck %s
+
+; CHECK: define void @f(ptr %p)
+; CHECK:   ret void
+define void @f(i32* %p) {
+  ret void
+}

llvm/tools/llvm-as/llvm-as.cpp

static void WriteOutputFile(const Module *M, const ModuleSummaryIndex *Index) {
   }
 
   // Declare success.
   Out->keep();
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
-  LLVMContext Context;
   cl::HideUnrelatedOptions(AsCat);
   cl::ParseCommandLineOptions(argc, argv, "llvm .ll -> .bc assembler\n");
+  LLVMContext Context;
 
   // Parse the file now...
   SMDiagnostic Err;
   auto SetDataLayout = [](StringRef) -> Optional<std::string> {
     if (ClDataLayout.empty())
       return None;
     return ClDataLayout;
   };

llvm/tools/llvm-dis/llvm-dis.cpp

 
 static ExitOnError ExitOnErr;
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
 
   ExitOnErr.setBanner(std::string(argv[0]) + ": error: ");
 
+  cl::ParseCommandLineOptions(argc, argv, "llvm .bc -> .ll disassembler\n");
+
   LLVMContext Context;
   Context.setDiagnosticHandler(
       std::make_unique<LLVMDisDiagnosticHandler>(argv[0]));
-  cl::ParseCommandLineOptions(argc, argv, "llvm .bc -> .ll disassembler\n");
 
   if (InputFilenames.size() < 1) {
     InputFilenames.push_back("-");
   } else if (InputFilenames.size() > 1 && !OutputFilename.empty()) {
     errs()
         << "error: output file name cannot be set for multiple input files\n";
     return 1;
   }

llvm/tools/opt/opt.cpp

 // main for opt
 //
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
 
   // Enable debug stream buffering.
   EnableDebugBuffering = true;
 
-  LLVMContext Context;
-
   InitializeAllTargets();
   InitializeAllTargetMCs();
   InitializeAllAsmPrinters();
   InitializeAllAsmParsers();
 
   // Initialize passes
   PassRegistry &Registry = *PassRegistry::getPassRegistry();
   initializeCore(Registry);

 
 #ifdef BUILD_EXAMPLES
   initializeExampleIRTransforms(Registry);
 #endif
 
   cl::ParseCommandLineOptions(argc, argv,
     "llvm .bc -> .bc modular optimizer and analysis printer\n");
 
+  LLVMContext Context;
+
   if (AnalyzeOnly && NoOutput) {
     errs() << argv[0] << ": analyze mode conflicts with no-output mode.\n";
     return 1;
   }
 
   // FIXME: once the legacy PM code is deleted, move runPassPipeline() here and
   // construct the PassBuilder before parsing IR so we can reuse the same
   // PassBuilder for print passes.