Refactor the scheduling predicates based on MCInstPredicate. In this case, AArch64InstrInfo::isScaledAddr().
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Hi Evandro.
Thanks for the patch. However, it is quite big to review.
Can you split it into multiple patches?
I think you can start by just rewiting method isScaledAddr.
It would be easier for me (and other reviewers) to see the differences in the auto-generated .inc file.
About isScaledAddr:
You can use a MCOpcodeSwitchStatement and rewrite that definition to something like this:
// Common variants to various AArch64 processor models. let FunctionMapper = "AArch64_AM::getMemExtendType" in def CheckExtendType : CheckImmOperand_s<3, "AArch64_AM::UXTX">; let FunctionMapper = "AArch64_AM::getMemDoShift" in def CheckFoldedShift : CheckImmOperandSimple<3>; // ScaledIdxPred is true if a WriteLDIdx operand will be // scaled. Subtargets can use this to dynamically select resources and // latency for WriteLDIdx and ReadAdrBase. // // Method 'isScaledAddr' Returns true if this is load/store scales or extends // its register offset. This refers to scaling a dynamic index as opposed to // scaled immediates. MI should be a memory op that allows scaled addressing. def ScaledIdxPred : TIIPredicate<"isScaledAddr", MCOpcodeSwitchStatement<[ MCOpcodeSwitchCase<[ LDRBBroW, LDRBroW, LDRDroW, LDRHHroW, LDRHroW, LDRQroW, LDRSBWroW, LDRSBXroW, LDRSHWroW, LDRSHXroW, LDRSWroW, LDRSroW, LDRWroW, LDRXroW, STRBBroW, STRBroW, STRDroW, STRHHroW, STRHroW, STRQroW, STRSroW, STRWroW, STRXroW, LDRBBroX, LDRBroX, LDRDroX, LDRHHroX, LDRHroX, LDRQroX, LDRSBWroX, LDRSBXroX, LDRSHWroX, LDRSHXroX, LDRSWroX, LDRSroX, LDRWroX, LDRXroX, STRBBroX, STRBroX, STRDroX, STRHHroX, STRHroX, STRQroX, STRSroX, STRWroX, STRXroX ], MCReturnStatement<CheckAny<[ CheckNot<CheckExtendType>, CheckFoldedShift ]>> >], MCReturnStatement<FalsePred> > >;
The resulting code expanded by tablegen should be more readable (opcodes appear as cases of a switch-stmt).
I sugges that you to split this NFC patch into multiple patches.
To start, you can send a patch that rewrites isScaledAddr only, and then verify that the auto-generated definition in the aarch64 gen-instrinfo .inc file is still semantically correct.
You should be able to add llvm-mca tests to verify that writes which require isScaledAddr to resolve their scheduling class are now correctly analyzed by llvm-mca.
-Andrea
Question: the code beginning at MCOpcodeSwitchStatement above cannot be used as a regular MCSchedPredicate too. If so, how can I avoid writing the same statement twice, since this condition is used both in AArch64InstrInfo.cpp and in AArch64Sched*.td?
You can use a TIIPredicate definition to model an MCSchedPredicate.
The MCOpcodeSwitchStatement used by the ScaledIdxPred definition from my previous comment is effectively defining the body of method "isScaledAddr".
ScaledIdxPred can be used in MCSchedPredicate definitions.
Example. In AArch64SchedExynosM1 you could have:
-def M1ReadAdrBase : SchedReadVariant<[SchedVar<ScaledIdxPred, [ReadDefault]>, - SchedVar<NoSchedPred, [ReadDefault]>]>; +def M1ReadAdrBase : SchedReadVariant<[ + SchedVar<MCSchedPredicate<ScaledIdxPred>, [ReadDefault]>, + SchedVar<NoSchedPred, [ReadDefault]> +]>;
A TIIPredicate used in a MCSchedPredicate definition is always expanded into a TII method call.
That being said, I am not sure if we are talking about the same thing...
Break up the original patch in 3 installments, one for each existing predicate: AArch64InstrInfo::isScaledAddr(), AArch64InstrInfo::hasShiftedReg(), AArch64InstrInfo::hasExtendedReg().
llvm/lib/Target/AArch64/AArch64SchedPredicates.td | ||
---|---|---|
61–62 ↗ | (On Diff #174990) | 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... |
65–67 ↗ | (On Diff #174990) | ScaledIdxPred should be defined as MCSchedPredicate<ScaleIdxFn>. 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. I wonder if this is the main reason why you think that using a MCOpcodeSwitchStatement in the definition of ScaledIdxBody is problematic ... |
llvm/lib/Target/AArch64/AArch64SchedPredicates.td | ||
---|---|---|
61–62 ↗ | (On Diff #174990) | I think that you can use a !listconcat operator to concatenate the opcodes from those two predicates. Something like this (I didn't verify that it works though...): def ScaledIdxFn : TIIPredicate<"isScaledAddr", MCOpcodeSwitchStatement<[ MCOpcodeSwitchCase< !listconcat(IsLoadRegOffsetPred.ValidOpcodes, IsStoreRegOffsetPred.ValidOpcodes), MCReturnStatement<CheckAny<[ CheckNot<CheckExtendType>, CheckFoldedShift ]>> >], MCReturnStatement<FalsePred> > >; I think it should work. It would let you use the switch statement. |
llvm/lib/Target/AArch64/AArch64SchedPredicates.td | ||
---|---|---|
61–62 ↗ | (On Diff #174990) | By the way. If !listconcat works for you (and you are happy with it), then you can use it to improve the other two NFC patches too. |
llvm/lib/Target/AArch64/AArch64SchedPredicates.td | ||
---|---|---|
61–62 ↗ | (On Diff #174990) | It seems to work. Will do. |