Do not construct std::string from nullptr

[georgthegreat]

Summary

While I am trying to forbid such usages systematically in https://reviews.llvm.org/D79427 / P2166R0 to C++ standard, this PR fixes this (definitelly incorrect) usage in llvm.

This code is unreachable, so it could not cause any harm

[nikic]

LGTM

[mclow.lists]

I would rather see:

llvm_unreachable("unexpected type");
return ""; // or string()

i.e, keep the call to unreachable.

[nikic]

llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp

1277

@mclow.lists There already is a call to llvm_unreachable here ^ so the second call to unreachable seems redundant.

[mclow.lists]

llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp

1277

I know there are compilers that will warn if you get to the end of a fn and there's no return statement there. Don't know how good clang is about analyzing all the paths through a function and determining that all of them lead to a return (and/or unreachable)

To make it easy on the compiler (and the reader), if I was writing this, I would move the default case to the end, and make the function look end look like this:

    else
      return "u32";
  default:
      break;
  }
  llvm_unreachable("unexpected type");
  return "";
}

1299

But I would also rewrite this chunk as:

if (static_cast<const NVPTXTargetMachine &>(TM).is64Bit())
    return useB4PTR ? "b64" : "u64";
else
    return useB4PTR ? "b32" : "u32";

But now I'm indulging in feature-creep :-) so you can ignore this.

[georgthegreat]

llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp

1277

This patch has been integrated into our monorepo and compiles just find with at least clang7 and clang10.

As all the branches of the switch above result in either return of llvm_unreachable invocation, I have considered the extra llvm_unreachable call redundant.

I do not have any string preferences here. @mclow.lists, what would you suggest after all the considerations?

1299

This is not the code I am familiar with, so I just wanted to fix the obvious compilation / assertion.

[georgthegreat]

Restore unreachable return statement, upon mclow.lists@ request.

[mclow.lists]

LGTM

[georgthegreat]

So, could someone, please, commit this PR? I have no write access to llvm.

[dblaikie]

In D87697#2295870, @mclow.lists wrote:

I would rather see:

llvm_unreachable("unexpected type");
return ""; // or string()

i.e, keep the call to unreachable.

I'd split the difference and keep the unreachable but drop the return - llvm_unreachable is annotated noreturn and all the compilers we use for LLVM are OK with/don't warn on: "non_void f1() { llvm_unreachable(...); }"

As for the possibility of relying on a switch with a default, where all cases and the default return or call a no-return function. I'd be OK with that, if it works for all the compilers LLVM supports - if there's prior art/examples of this in the LLVM codebase, that'd be adequate for me.

& yeah, I'd be OK with @mclow.lists 's suggestion to move the default to the end and just have it break, keeping a singular unreachable (& though I'd favor fairly strongly not having a return after that) outside the switch.

[georgthegreat]

Changed as dblaikie@ suggested. Not all the types are handled during the switch, so default is still required in order to prevent compiler from issuing warnings.

[dblaikie]

Good for me - please wait for @mclow.lists to weigh in/close the loop as well, though.

[mclow.lists]

I'm fine with this.
This is not how I'd write it, but that's ok :-)

[georgthegreat]

@dblaikie, so, could you, please, commit this at last?

[dblaikie]

In D87697#2376849, @georgthegreat wrote:

@dblaikie, so, could you, please, commit this at last?

Oh, sure thing! Should be committed shortly.

llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp

report_fatal_error("Bad address space found while emitting PTX: " +
                        llvm::Twine(AddressSpace));
     break;
   }
 }
 
 std::string
 NVPTXAsmPrinter::getPTXFundamentalTypeStr(Type *Ty, bool useB4PTR) const {
   switch (Ty->getTypeID()) {
-  default:
-    llvm_unreachable("unexpected type");
-    break;
   case Type::IntegerTyID: {
     unsigned NumBits = cast<IntegerType>(Ty)->getBitWidth();
     if (NumBits == 1)
       return "pred";
     else if (NumBits <= 64) {
       std::string name = "u";
       return name + utostr(NumBits);
     } else {
       llvm_unreachable("Integer too large");
       break;
     }
     break;
   }
   case Type::HalfTyID:
     // fp16 is stored as .b16 for compatibility with pre-sm_53 PTX assembly.
     return "b16";
   case Type::FloatTyID:
     return "f32";
   case Type::DoubleTyID:
     return "f64";
   case Type::PointerTyID:
     if (static_cast<const NVPTXTargetMachine &>(TM).is64Bit())
       if (useB4PTR)
         return "b64";
       else
         return "u64";
     else if (useB4PTR)
       return "b32";
     else
       return "u32";
+  default:
+    break;
   }
   llvm_unreachable("unexpected type");
-  return nullptr;
 }
 
 void NVPTXAsmPrinter::emitPTXGlobalVariable(const GlobalVariable *GVar,
                                             raw_ostream &O) {
   const DataLayout &DL = getDataLayout();
 
   // GlobalVariables are always constant pointers themselves.
   Type *ETy = GVar->getValueType();