[PATCH] D154506: [ARM] in LowerConstantFP, make sure we cover armv6-m execute-only

[Ties Stuij via Phabricator via llvm-commits]

stuij marked an inline comment as done.
stuij added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:7074-7075
+    // we shouldn't trigger this for v6m execute-only
+    if (ST->isThumb1Only() && !ST->hasV8MBaselineOps())
+      llvm_unreachable("Unexpected architecture");
+
----------------
dmgreen wrote:
> Perhaps just use an assert instead of a llvm_unreachable? It's common to add the message to asserts in LLVM code.
According to the documentation, llvm_unreachable is preferable to an `assert(0)`, which it effectively is in this case:
https://llvm.org/docs/CodingStandards.html#assert-liberally

There was a longish thread discussing this, and I still came away with unreachable being preferred.
https://discourse.llvm.org/t/llvm-unreachable-is-widely-misused/60587


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154506