[PATCH] D152795: [ARM] generate armv6m eXecute Only (XO) code

[Ties Stuij via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/ARM/ARMPredicates.td:227-228
 def GenExecuteOnly : Predicate<"Subtarget->genExecuteOnly()">;
+def GenT1ExecuteOnly : Predicate<"Subtarget->genT1ExecuteOnly()">;
+def DontGenT1ExecuteOnly : Predicate<"!Subtarget->genT1ExecuteOnly()">;
 
----------------
john.brawn wrote:
> stuij wrote:
> > john.brawn wrote:
> > > I don't think having a genT1ExecuteOnly predicate is a good idea, as it combines a bunch of things which makes things less clear when looking at the tablegen files. I think what we need is:
> > >  * DontGenExecuteOnly which is just the negation of GenExecuteOnly
> > >  * Patterns that are `Requires<[GenT1ExecuteOnly]>` are instead `Requires<[IsThumb1Only, GenExecuteOnly, DontUseMovt]>`
> > >  * tLDRLIT_ga_abs should have `Requires<[IsThumb, DontUseMovt, DontGenExecuteOnly]>`
> > The reason why I decided to create GenT1ExecuteOnly, was actually because UseMovt itself is using a bunch of things which make sense for the specific use case of using MOVT, but it can't be used for just the negation of the MOVT instruction being there:
> > 
> >     bool ARMSubtarget::useMovt() const {
> >       // NOTE Windows on ARM needs to use mov.w/mov.t pairs to materialise 32-bit
> >       // immediates as it is inherently position independent, and may be out of
> >       // range otherwise.
> >       return !NoMovt && hasV8MBaselineOps() &&
> >              (isTargetWindows() || !OptMinSize || genExecuteOnly());
> >     }
> > 
> > It becomes confusing to use `useMovt` in for example this case where people will conflate useMovt with the MOVT instruction being there or not. And in this case, we're pulling things in that we might not. Things like `isTargetWindows` aren't relevant here and genXO is orthogonal in our use case.
> > 
> > I would myself prefer being clear about our intent: generating XO for T1, which is captured by `GenT1ExecuteOnly`. It's not obvious to someone looking at the code that the clauses which make up the decision for generating t1 code are related (I bet not everyone knows the check for armv8m.base is there because it is mostly T1, but does include movw/movt). Just looking at the predicate list doesn't make these details clear.
> Is see. In that case keeping GenT1ExecuteOnly is fine, but I think the logic should move into the predicate itself, i.e.
> 
> ```
> def GenT1ExecuteOnly : Predicate<"Subtarget->genExecuteOnly() && Subtarget->isThumb1Only() && !Subtarget->hasV8MBaselineOps()>"
> ```
done!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152795