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

[Dave Green via Phabricator via llvm-commits]

dmgreen 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");
+
----------------
stuij wrote:
> 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
I meant:
```
assert((!ST->isThumb1Only() || ST->hasV8MBaselineOps()) && "Unexpected architecture");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154506