[PATCH] D54777: [AArch64] Refactor the scheduling predicates (1/3) (NFC)

[Evandro Menezes via Phabricator via llvm-commits]

evandro marked 3 inline comments as done.
evandro added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64SchedPredicates.td:61-62
+// with an extended or scaled register.
+def ScaledIdxBody : CheckAll<[CheckAny<[IsLoadRegOffsetPred,
+                                        IsStoreRegOffsetPred]>,
+                              CheckAny<[CheckNot<CheckMemExtLSL>,
----------------
andreadb wrote:
> If `IsLoadRegOffsetPred` and `IsStoreRegOffsetPred` are only used by the definition of `ScaledIdxBody`, then you should simply use a `MCOpcodeSwitchStatement`.
> 
> I had a look at your three NFC patches; this is the only place where you use `IsLoadRegOffsetPred` and `IsStoreRegOffsetPred`. 
> 
> I know that in one of your previous comments you mentioned a prolem with `MCOpcodeSwitchStatement`. I still don't understand what the problem is. When I tried to rewrite your patch using `MCOpcodeSwitchStatement` everything worked fine for me...
> I wonder if the issue is caused by the way you defined `ScaledIdxPred`. That definition should be changed to reference `ScaledIdxFn` directly (See my comment below).
Yes, I intend to reuse both `IsLoadRegOffsetPred` and `IsStoreRegOffsetPred` in a future patch.  This is the reason; nothing wrong with using a more elegant switch statement in the emitted code.


================
Comment at: llvm/lib/Target/AArch64/AArch64SchedPredicates.td:65-67
+def ScaledIdxPred : MCSchedPredicate<ScaledIdxBody>;
+def ScaledIdxFn   : TIIPredicate<"isScaledAddr",
+                                 MCReturnStatement<ScaledIdxBody>>;
----------------
andreadb wrote:
> `ScaledIdxPred` should be defined as `MCSchedPredicate<ScaleIdxFn>`.
> Otherwise, you would end up expanding the entire body of 'isScaledAddr` inline; it won't be expanded into a TII call to `isScaledAddr()`.
> 
> So, the definitions should be:
> ```
> def ScaledIdxFn   : TIIPredicate<"isScaledAddr",
>                                  MCReturnStatement<ScaledIdxBody>>;
> def ScaledIdxPred : MCSchedPredicate<ScaledIdxFn>;
> ```
> 
> As I mentioned in one of my comments from yesterday, a `TIIPredicate` //is-a// `MCInstPredicate`.
> When used in the defintion of a `MCSchedPredicate`, it is expanded by the PredicateExpander (see llvm/utils/Tablegen/PredicateExpander.{h,cpp}) into a TII call. 
> 
> I wonder if this is the main reason why you think that using a `MCOpcodeSwitchStatement` in the definition of `ScaledIdxBody` is problematic ...
Neat!


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

https://reviews.llvm.org/D54777