[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;
----------------
paquette wrote:
> 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?
Yeah, RegOrConstant.


================
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) {}
----------------
paquette wrote:
> 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.
Ah yeah. In that case, I think it's still better to use extra memory and store both a Register and an in64_t, rather than casting.


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

https://reviews.llvm.org/D97731