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

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/Utils.h:263
 
+class RegOrCst {
+  int64_t Val;
----------------
aemerson wrote:
> 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.
Like rename it to Constant?


================
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) {}
----------------
aemerson wrote:
> 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
> };
> ```
I actually wrote this as a union originally, but it would do the wrong thing if Register has a non-trivial destructor or non-trivial copy constructor, so I just avoided the whole issue.


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

https://reviews.llvm.org/D97731