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

[Prakhar Bahuguna via Phabricator via llvm-commits]

prakhar added inline comments.


================
Comment at: lib/Target/ARM/ARM.td:267
+// Execute-only is also referred to as pure code by GCC
+def FeatureExecuteOnly : SubtargetFeature<"execute-only", "GenExecuteOnly", "true",
+                                        "Don't use data access to code sections">;
----------------
rengolin wrote:
> grosbach wrote:
> > Why use a Feature flag for this? This isn't an attribute of the CPU, but rather a code generation option, right?
> > 
> > Would a function attribute be sufficient? At first look, it seems all the decisions to be made are about how to handle any given function's constant data. That would have the added benefit of making LTO "just work" since function attributes propagate naturally.
> Oh, I missed that one! Indeed, this is a codegen attribute, which is dependent on the platform, but not enough to be a sub-target feature.
> 
> I think the confusion is that Clang emits "-target-feature" "+execute-only", but those are different things.
If I understand the concept correctly, then it sounds like execute-only is something that relates to codegen rather than an attribute of a CPU. How would I go about implementing a codegen option, and is there an example I can follow for this?


================
Comment at: lib/Target/ARM/ARMSubtarget.cpp:173
+  // Execute only support requires movt support
+  if (genExecuteOnly()) {
+    NoMovt = false;
----------------
grosbach wrote:
> 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.
It's possible to reach this point with llc/llvm-mc by specifying +execute-only and +no-movt for -target-feature. As I'll be adding clang diagnostics for -mno-movt being enabled alongside -mexecute-only, I'll revert this back to an assert.


https://reviews.llvm.org/D27449