[PATCH] D101349: AArch64: support i128 cmpxchg in GlobalISel.

[Jessica Paquette via Phabricator via llvm-commits]

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