[PATCH] D134336: [AArch64] Lower multiplication by a constant int to madd

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5922
+        BuildMI(MF, Root.getDebugLoc(), TII->get(MovI->Opcode), NewVR)
+            .addImm(MovI->Op1)
+            .addImm(MovI->Op2);
----------------
Allen wrote:
> efriedma wrote:
> > Allen wrote:
> > > efriedma wrote:
> > > > Sorry, missed an issue here the first time I looked at this...
> > > > 
> > > > The current complete set of opcodes that immediate expansion can produce is listed in AArch64ExpandPseudo::expandMOVImm .  If the opcode is movz/movn, there are two immediate operands.  But the opcode can also be ORRWri/ORRXri; in that case, there a register operand (wzr/xzr), and an immediate operand.
> > > > 
> > > > I think this comes up for something like `int f(int a) { return a * 44 + 16773120; }`.
> > > Despite  the fact that movz/movn have two immediate operands, but it will generate a mov as alias, which only has one  immediate operand.
> > > see the following code frag in AArch64_IMM::expandMOVImm.
> > > ```
> > >   // Prefer MOVZ/MOVN over ORR because of the rules for the "mov" alias.
> > >   if ((BitSize / 16) - OneChunks <= 1 || (BitSize / 16) - ZeroChunks <= 1) {
> > >     expandMOVImmSimple(Imm, BitSize, OneChunks, ZeroChunks, Insn);
> > >     return;
> > >   }
> > > ```
> > The first instruction returned by expandMOVImm is always "mov", yes.  "mov" is an alias for one of three instructions: movz, movn, and orr.  (See section C3.3.4 in the ARM architecture reference.)
> > 
> > As far as I can tell, the operands you're adding are correct for movz and movn, but not orr.
> Done, revert to use orr if valid,thanks
Do we have test coverage for any constants like 16773120?

Instead of calling processLogicalImmediate like this, it would be more intuitive to explicitly check that MovI->Opcode is the opcode we expect.  The current code works, but it's tightly tied to the exact implementation of expandMOVImm.


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

https://reviews.llvm.org/D134336