[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:
> > 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");
> > ```
> Yes, I get that, but reading the documentation:
> 
> "Use llvm_unreachable to mark a specific point in code that should never be reached. This is especially desirable for addressing warnings about unreachable branches, etc., but can be used whenever reaching a particular code path is unconditionally a bug (not originating from user input; see below) of some kind."
> 
> To me it reads that `llvm_unreachable` is preferable here.
I don't think assert with a condition is discouraged, only `assert(false)` or `assert(0)`. Those are better as `llvm_unreachable`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154506