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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb 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>,
----------------
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).


================
Comment at: llvm/lib/Target/AArch64/AArch64SchedPredicates.td:65-67
+def ScaledIdxPred : MCSchedPredicate<ScaledIdxBody>;
+def ScaledIdxFn   : TIIPredicate<"isScaledAddr",
+                                 MCReturnStatement<ScaledIdxBody>>;
----------------
`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 ...


https://reviews.llvm.org/D54777