[PATCH] Add support for ARM modified immediate syntax

[Mihail Popa]

Tim,

regarding disassembly - I can add that in, it's rather simple.
Having that, may I commit as first step in the incremental solution?

Now regarding codegen changes and the larger question of what makes sense
architecturally:

1. initial proposal is to change the text assembler to support a very
peculiar corner-case syntax
2. that spilled over to affecting the compiled code assembler
3. that spilled over to affecting the frame lowering and the
MachineInstruction representation
(a higher order abstraction)
4. that spilled over to affecting the instruction selection process and the
structure of the Selection DAG
(an even higher order abstraction)

I simply fail to grasp the engineering sense of modifying instruction
selection to accommodate a corner
case in text assembly syntax. Once you change something fundamental at
instruction selection level,
everything that follows downstream is potentially affected. A change so
fundamental as this
(representing an immediate as two immediates, but only for some
instructions) can potentially bring a
couple of years of bugfixing. This is such a major change that it basically
nullifies all validation done up
to this point.

Can you still guarantee that the register allocator won't crash? Can you
still guarantee that it'll do it's
job properly? Can you guarantee that there will be no performance hits? I
don't think anyone can.

I strongly believe in separating the compilation processes into stages with
hard boundaries, and one such
hard boundary should be between assembler and the backend. The fact that
they lay in the same process
is irrelevant. We should develop as-if these were different projects worked
on by different teams.

> I think that if we're not making changes to LLVM because of the risk
> then the project has very serious problems, beyond being able to
> assemble weird edge cases of ARM instructions.

I have seen this go wrong so many times that I learnt by painful experience
to be cautious. I don't think an
engineering project should be led by policies and ideals - this leads to
lack of design and architecture and
ultimately makes it unmaintainable. Quite the contrary.. it should be a
pragmatic effort into finding the best
compromises.

Now I agree with you that copy-pasting 7 instruction definitions is ugly,
and I'm not particularly proud of it.
But at least each of those copy pastes has it's own unit test, and more
importantly, each of the copy
pastes can be tested in isolation from the others. You cannot claim the
same about changing instruction
selection. It has far reaching implications that go beyond the scope of
unit testing, maybe beyond
the of testing.

This would be one of those things that you can only "check" by running
every program you can get through
the compiler. When you reach that point, the project is in danger. You
cannot rely on testing to expose bugs.
Testing is simply the last safety net. You should rely on architecture,
proper engineering and proper review.
So here is one meaning of "best compromise": don't do anything that makes
you fully dependent on testing.

And then you mention the question of risk. Whose risk is this? It's
definitely not yours. There is some of my
risk because I'm making the changes and I am ultimately responsible for the
consequences, not the
reviewer. But there are also people using LLVM in their own projects.. what
about their risk? This is the true
risk: breaking something for someone using LLVM in a custom solution just
because we
decided to be cowboys. This is the real risk and it isn't ours.. as such we
should not take it.

Regards,
Mihai


On Thu, Aug 15, 2013 at 10:16 AM, Tim Northover <t.p.northover at gmail.com>wrote:

> Hi Mihai,
>
> > The effort is not the issue here. Do you really think it's work the risk?
>
> I think that if we're not making changes to LLVM because of the risk
> then the project has very serious problems, beyond being able to
> assemble weird edge cases of ARM instructions.
>
> I've seen projects that get like that, and it's not fun.
>
> > Do you really think this is a better and neater solution?
>
> I do. I think it would be a change worth making even without this work
> in the same area motivating it. In fact I'm seriously considering
> taking a stab at it myself this weekend.
>
> > If you do, I'll do it - but I must say that the original patch I
> > submitted is the solution that makes more
> > sense simply because it keeps changes local.
>
> How about doing this incrementally, then? I think a key question your
> patch doesn't address is how disassembly will be supported. I've
> mentioned it before, but these two lines should not produce the same
> output (it'll come up in MC Hammer eventually):
>
> $ echo 0x04,0x00,0x90,0xe2 | bin/llvm-mc -disassemble -arch=arm
>     adds r0, r0, #4
> $ echo 0x01,0x0f,0x90,0xe2 | bin/llvm-mc -disassemble -arch=arm
>     adds r0, r0, #4
>
> I think that the best solution to that problem would involve an
> instruction with essentially the form I'm advocating (modulo edge
> details like bit-packed imm or two imms and so on). The existing
> instructions (ADDri etc) would probably become isCodeGenOnly instead
> of the new ones (ADDrii etc) being isAsmParserOnly. So why don't we
> plan this in three phases:
>
> 1. Commit your patch (or something similar).
> 2. Add disassembly support to your new instructions, switch
> isAsmParserOnly to isCodeGenOnly on the old ones.
> 3. Go through each use in CodeGen, adapting them to use the new
> instruction and remove the legacy ones. We can be sure all uses have
> been fixed if we remove the instructions entirely; attempted use will
> fail at compile-time.
>
> Even the names actually work out nicely. I don't think ADDrii is
> significantly worse than ADDri for those instructions, particularly as
> it would remind people who BuildMI them in future that they need to be
> careful.
>
> Cheers.
>
> Tim.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130815/fcab3be3/attachment.html>