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

[Ties Stuij via Phabricator via llvm-commits]

stuij added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:3962
+  // use immediate relocations.
+  if (Subtarget->useMovt() || Subtarget->genT1ExecuteOnly()) {
+    if (Subtarget->useMovt())
----------------
john.brawn wrote:
> This should be genExecuteOnly, as it's not thumb1-specific that we want to avoid generating a constant pool, it's just that up until now execute-only implied useMovt.
right, thanks! I'm guessing you meant just changing `genT1ExecuteOnly` to `genExecuteOnly`, as we still want to generate MOVW/MOVT for efficiency.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb.td:1613
+let isReMaterializable = 1, isMoveImm = 1, Size = 16, hasNoSchedulingInfo = 1 in
+def t1MOVi32imm : PseudoInst<(outs rGPR:$dst), (ins i32imm:$src), NoItinerary,
+                            [(set rGPR:$dst, (i32 imm:$src))]>,
----------------
john.brawn wrote:
> This should be tMOVi32imm for consistency with everything else (all other 16-bit thumb instructions, or pseudoinstructions that expand to 16-bit thumb instructions, are named tSomething).
Thanks. Yes, I had that initially, but then opted for t1, as it's just clearer. I do agree in hindsight that following convention is better here.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152795