[PATCH] D101349: AArch64: support i128 cmpxchg in GlobalISel.
Jessica Paquette via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 27 09:53:34 PDT 2021
paquette added a comment.
I think it would be good to add dedicated legalizer + regbankselect MIR tests?
================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:2553
// Check we have the right regbank always.
assert(SrcRB.getID() == AArch64::FPRRegBankID &&
DstRB.getID() == AArch64::FPRRegBankID &&
----------------
It seems like this assert can probably be replaced with something like
```
assert(SrcRB.getID() == DstRB.getID() && "Src and dst register banks must match!");
```
And then moved above the `if (SrcRB.getID() == AArch64::GPRRegBankID...`
================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:1059
+ MachineRegisterInfo &MRI,
+ LegalizerHelper &Helper) const {
+ MachineIRBuilder &MIRBuilder = Helper.MIRBuilder;
----------------
Some comments on the various cases would be nice here? (e.g. LSE, no LSE, etc.)
================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64RegisterBankInfo.cpp:880
LLT SrcTy = MRI.getType(MI.getOperand(1).getReg());
- if (SrcTy.getSizeInBits() == 128) {
+ if (MRI.getRegClassOrNull(Src) == &AArch64::XSeqPairsClassRegClass) {
+ OpRegBankIdx[0] = PMI_FirstGPR;
----------------
I guess this is just for the s128 case, so maybe this can be refactored a little
```
...
if (SrcTy.getSizeInBits() != 128)
break;
auto Idx = (MRI.getRegClassOrNull(Src) == &AArch64::XSeqPairsClassRegClass) ? PMI_FirstGPR : PMI_FirstFPR;
...
```
Can't think of a better way to decide this without checking the register class directly, unfortunately.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101349/new/
https://reviews.llvm.org/D101349
More information about the llvm-commits
mailing list