Hi Paul,
First, I'm not sure it's ok for me to review this here (rather than
llvm-commits), but I'm going to copy the other list as well so other
people can look at it too. Feel free to remove llvm-dev if not
appropriate, anyone.
On 27 November 2010 21:17, Paul Curtis <plc at rowley.co.uk>
wrote:> Attached is a patch for the ARM target. The ARM v7M profile does not
> have the signed most-significant-word multiply instruction so SMMUL,
> for instance, is not valid on Cortex-M3 and Cortex-M4.
This is not entirely correct. Cortex-M4 does have it (ARMv7E-M, ARM
ARM v7M item A7.7.142).
> The attached patch adds an additional attribute, +mmul, which
> controls most-significant word multiplies on v6T2+ targets.
This is not the only instruction that v7M doesn't have that others do
(including v7EM), so would be good to have a flag that takes care of
all of them, rather than one specifically for SMMUL. You could call
that flag, for instance, hasV6SIMD or something of the sort, for this
group of instructions (QADD, QSUB, SMLAD, SMLSD, etc).
> This is especially important for me now that I've started to benchmark
> clang+llvm against GCC for some of my integer mathematical functions.
I'm not an expert in table gen, but the patch looks fine to me as a
start. I think it's ok for you to call the flag in a generic way even
if you just implement SMMUL, so later, it'd be easier for other people
(or yourself) to just change the lowering part for the other
instructions.
Have you ran all tests after your changes? Do you have a test case (a
few lines that should never generate SMMUL for a v7M core, so you can
add to the LLVM test dir?
thanks for the patch!
--renato