OpaquePtr: Support i32** with --force-opaque-pointers

[dexonsmith]

Summary

4506f614cb6983a16d117cf77a968608e66d7a5c fixed parsing of textual IR to
reject ptr*, but broke the auto-conversion of i32** to ptr with
--force-opaque-pointers.

Get that working again by refactoring LLParser::parseType to only send
ptr-spelled pointers into the type suffix logic when it's the return
of a function type. This also rejects ptr addrspace(3) addrspace(2),
which 1e6303e60ca5af4fbe7ca728572fd65666a98271 invadvertently started
accepting. Just the default top-level error message for the
double-addrspace since I had trouble thinking of something nice; open to
ideas, but it's fine as is (it doesn't look valid the way that ptr*
does).

[dexonsmith]

llvm/lib/AsmParser/LLParser.cpp

2630–2645

Actually, maybe this is all wrong. ptr addrspace(1) addrspace(2) isn't valid. This would have been rejected originally, but was loosened at the same time that ptr * slipped through.

I'll undo the addrspace logic shuffle, and update this patch to fix that bug too.

[dblaikie]

Looks good to me!

[dexonsmith]

In D105146#2848285, @dblaikie wrote:

Looks good to me!

Thanks for the review! After noticing the addrspace problem I rewrote the patch and added more tests; PTAL when you get a chance.

[nikic]

LGTM, thanks!

llvm/lib/AsmParser/LLParser.cpp

if (!Entry.first) {
       Entry.second = Lex.getLoc();
     }
     Result = Entry.first;
     Lex.Lex();
     break;
   }
   }
 
+  // Handle (explicit) opaque pointer types (not --force-opaque-pointers).
+  //
+  // Type ::= ptr ('addrspace' '(' uint32 ')')?
   if (Result->isOpaquePointerTy()) {
     unsigned AddrSpace;
     if (parseOptionalAddrSpace(AddrSpace))
       return true;
     Result = PointerType::get(getContext(), AddrSpace);
+
+    // Give a nice error for 'ptr*'.
+    if (Lex.getKind() == lltok::star)
+      return tokError("ptr* is invalid - use ptr instead");
+
+    // Fall through to parsing the type suffixes only if this 'ptr' is a
+    // function return. Otherwise, return success, implicitly rejecting other
+    // suffixes.
+    if (Lex.getKind() != lltok::lparen)
+      return false;
   }
 
   // parse the type suffixes.
   while (true) {
     switch (Lex.getKind()) {
     // End of type.
     default:
       if (!AllowVoid && Result->isVoidTy())
         return error(TypeLoc, "void type only allowed for function results");
       return false;
 
     // Type ::= Type '*'
     case lltok::star:
       if (Result->isLabelTy())
         return tokError("basic block pointers are invalid");
       if (Result->isVoidTy())
         return tokError("pointers to void are invalid - use i8* instead");
-      if (Result->isOpaquePointerTy())
-        return tokError("ptr* is invalid - use ptr instead");
       if (!PointerType::isValidElementType(Result))
         return tokError("pointer to this type is invalid");
       Result = PointerType::getUnqual(Result);
       Lex.Lex();
       break;
 
     // Type ::= Type 'addrspace' '(' uint32 ')' '*'
     case lltok::kw_addrspace: {
       if (Result->isLabelTy())
         return tokError("basic block pointers are invalid");
       if (Result->isVoidTy())
         return tokError("pointers to void are invalid; use i8* instead");
       if (!PointerType::isValidElementType(Result))
         return tokError("pointer to this type is invalid");
       unsigned AddrSpace;
       if (parseOptionalAddrSpace(AddrSpace) ||
           parseToken(lltok::star, "expected '*' in address space"))
         return true;
 
       Result = PointerType::get(Result, AddrSpace);
       break;
     }
 
     /// Types '(' ArgTypeListI ')' OptFuncAttrs
     case lltok::lparen:
       if (parseFunctionType(Result))
         return true;
       break;
     }
   }

llvm/test/Assembler/invalid-opaque-ptr-double-addrspace.ll

+; RUN: not llvm-as < %s -disable-output 2>&1 | FileCheck %s
+
+; CHECK: expected top-level entity
+@g1 = external global ptr addrspace(3) addrspace(4)

llvm/test/Assembler/opaque-ptr.ll

 ; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis | FileCheck %s
 ; RUN: verify-uselistorder %s
 
 ; CHECK: @global = external global ptr
 @global = external global ptr
 
+; CHECK: @fptr1 = external global ptr ()*
+; CHECK: @fptr2 = external global ptr () addrspace(1)*
+; CHECK: @fptr3 = external global ptr () addrspace(1)* addrspace(2)*
+@fptr1 = external global ptr ()*
+@fptr2 = external global ptr () addrspace(1)*
+@fptr3 = external global ptr () addrspace(1)* addrspace(2)*
+
 ; CHECK: define ptr @f(ptr %a) {
 ; CHECK:     %b = bitcast ptr %a to ptr
 ; CHECK:     ret ptr %b
 define ptr @f(ptr %a) {
     %b = bitcast ptr %a to ptr
     ret ptr %b
 }
 

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

;
   %a = alloca i17
   call void @fn.fwd(i32 0)
   store i32 0, i32* @g.fwd
   ret void
 }
 
 @g.fwd = global i32 0
 declare void @fn.fwd(i32)
+
+define void @f2(i32** %p) {
+; CHECK-LABEL: define {{[^@]+}}@f2(
+; CHECK-SAME: ptr {{%.*}}) {
+  unreachable
+}
+
+define void @f3(i32 addrspace(1)* addrspace(2)* %p) {
+; CHECK-LABEL: define {{[^@]+}}@f3(
+; CHECK-SAME: ptr addrspace(2) {{%.*}}) {
+  unreachable
+}