[PATCH] D83404: [PowerPC][PCRelative] Thread Local Storage Support for Local Exec

[Kamau Bridgeman via Phabricator via llvm-commits]

kamaub added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp:424
+      default:
+        report_fatal_error("Unsupported Modifier for fixup_ppc_imm34.");
+      case MCSymbolRefExpr::VK_TPREL:
----------------
MaskRay wrote:
> Is this unreachable? If yes, assert may be better
This switch was originally commit in rGbd2068031121adf5a0e28d9306a1741d6f0bbd87 but was reverted by rGce1e4853b5a15d679bd662ac5777a2390daf0391 because of failures in Release+Asserts mode with an assert. So rG97470897c436a6a5d682fb8ab296d0bcdc6e32a4 committed it as an `report_fatal_error()` to resolve the problem. I am not sure that `assert()` won't cause a similar problem as `llvm_unreable()` since we replaced `llvm_unreachable()` with `report_fatal_error()` for similar reason to why they suggest using `assert()` with `llvm_unreachable()` in 
> https://llvm.org/docs/CodingStandards.html#assert-liberally:
> This has a few issues, the main one being that some compilers might not understand the assertion, or warn about a missing return in builds where assertions are compiled out.
> 
> Today, we have something much better: llvm_unreachable:

I could be wrong though, should we still replace it with an assert? I think the answer to your question is yes, it was reachable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83404/new/

https://reviews.llvm.org/D83404