[PATCH] D27449: [ARM] Implement execute-only support in CodeGen

[Jim Grosbach via Phabricator via llvm-commits]

grosbach added inline comments.


================
Comment at: lib/Target/ARM/ARMSubtarget.cpp:173
+  // Execute only support requires movt support
+  if (genExecuteOnly()) {
+    NoMovt = false;
----------------
rengolin wrote:
> grosbach wrote:
> > rengolin wrote:
> > > grosbach wrote:
> > > > prakhar wrote:
> > > > > rengolin wrote:
> > > > > > I'm worried about the silent behaviour here.
> > > > > > 
> > > > > > If movt is disabled by some other part or user flag, than this should be an error, not a silent change.
> > > > > Yes, this should be an error. I've changed this to report an error if NoMovt is enabled or the target doesn't support MOVT. I'll also add a diagnostic to clang for this.
> > > > The error should be detected earlier and a proper diagnostic issued if at all possible. Using report_fatal_error() for user-facing diagnostics is an option of last resort and an indicator of a layering failure elsewhere.
> > > There is an error in Clang (D27450) that detects this. Is there a better way to do this in LLVM itself?
> > If there's a legitimate user-facing way to get here and this be the first time we can diagnose the error, we can use the emitError() method on the LLVMContext to complain about it. If, OTOH, getting here in that state indicates a bug earlier in the pipeline (probably true?), then perhaps this could just be an assert().
> There shouldn't be a user-facing way to get here, though it may be possible possible with the right combination of flags on llc/llvm-mc.
> 
> A quick assert would be of the same value as the one above, I'm ok with that, too.
SGTM. I don't think it's too horrible if llc/llvm-mc can trigger asserts. Not ideal, but those aren't (supposed to be) user-facing tools at all.


https://reviews.llvm.org/D27449