[PATCH] D97731: [AArch64][GlobalISel] Lower G_BUILD_VECTOR -> G_DUP

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/Utils.h:263
 
+class RegOrCst {
+  int64_t Val;
----------------
A quick comment to explain what this is for would be helpful. I also think we have the space to fully spell out Constant in the type.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/Utils.h:267
+public:
+  explicit RegOrCst(Register Reg) : Val(Reg), IsReg(true) {}
+  explicit RegOrCst(int64_t Cst) : Val(Cst), IsReg(false) {}
----------------
This is implementing a tagged union, and since it's better to not try to cast a Register to an int64 here, try:

```
class RegOrConstant {
  union {
    int64_t IntVal;
    Register Reg;
  };
  bool IsReg;
public:
  explicit RegOrConstant(int64_t Cst) : IntVal(Cst), IsReg(false) {}
  explicit RegOrConstant(Register R) : Reg(R), IsReg(true) {}
  Register getReg()...
... etc
};
```


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64GlobalISelUtils.h:23
+class MachineRegisterInfo;
+class RegOrCst;
 namespace AArch64GISelUtils {
----------------
The types used in the declarations aren't opaque pointers, so forward declaring doesn't help here.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64GlobalISelUtils.h:34
+/// Checks for generic opcodes and AArch64-specific generic opcodes.
+Optional<RegOrCst> getAArch64VectorSplat(const MachineInstr &MI,
+                                         const MachineRegisterInfo &MRI);
----------------
These helpers should be names could be more precise, because they seem to return a scalar, whereas "splat" usually refers to the vector itself. Something like `getAArch64VectorSplatSrc()` or `getAArch64VectorSplatScalar()` might be better.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerLowering.cpp:707
+  // Don't block tablegen-imported patterns for all-zero and all-one build
+  // vectors.
+  int64_t Cst = Splat->getCst();
----------------
I'd specifically call out the `immAllZeroesV` and `immAllOnesV` patterns in this comment.


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

https://reviews.llvm.org/D97731