<div dir="ltr">Tim,<div><br></div><div>regarding disassembly - I can add that in, it's rather simple.</div><div>Having that, may I commit as first step in the incremental solution?</div><div><br></div><div>Now regarding codegen changes and the larger question of what makes sense </div>
<div>architecturally:</div><div><br></div><div>1. initial proposal is to change the text assembler to support a very peculiar corner-case syntax</div><div>2. that spilled over to affecting the compiled code assembler</div>
<div>3. that spilled over to affecting the frame lowering and the MachineInstruction representation </div><div>(a higher order abstraction)</div><div>4. that spilled over to affecting the instruction selection process and the structure of the Selection DAG</div>
<div>(an even higher order abstraction)</div><div><br></div><div>I simply fail to grasp the engineering sense of modifying instruction selection to accommodate a corner</div><div>case in text assembly syntax. Once you change something fundamental at instruction selection level,</div>
<div>everything that follows downstream is potentially affected. A change so fundamental as this</div><div>(representing an immediate as two immediates, but only for some instructions) can potentially bring a</div><div>couple of years of bugfixing. This is such a major change that it basically nullifies all validation done up</div>
<div>to this point.</div><div><br></div><div>Can you still guarantee that the register allocator won't crash? Can you still guarantee that it'll do it's</div><div>job properly? Can you guarantee that there will be no performance hits? I don't think anyone can.</div>
<div><br></div><div>I strongly believe in separating the compilation processes into stages with hard boundaries, and one such</div><div>hard boundary should be between assembler and the backend. The fact that they lay in the same process</div>
<div>is irrelevant. We should develop as-if these were different projects worked on by different teams.<br></div><div><br></div><div><div><span style="font-family:arial,sans-serif;font-size:13px">> I think that if we're not making changes to LLVM because of the risk</span><br style="font-family:arial,sans-serif;font-size:13px">
<span style="font-family:arial,sans-serif;font-size:13px">> then the project has very serious problems, beyond being able to</span><br style="font-family:arial,sans-serif;font-size:13px"><span style="font-family:arial,sans-serif;font-size:13px">> assemble weird edge cases of ARM instructions.</span><br style="font-family:arial,sans-serif;font-size:13px">
</div><div><br></div><div>I have seen this go wrong so many times that I learnt by painful experience to be cautious. I don't think an </div><div>engineering project should be led by policies and ideals - this leads to lack of design and architecture and </div>
<div>ultimately makes it unmaintainable. Quite the contrary.. it should be a pragmatic effort into finding the best </div><div>compromises. </div></div><div><br></div><div>Now I agree with you that copy-pasting 7 instruction definitions is ugly, and I'm not particularly proud of it.</div>
<div>But at least each of those copy pastes has it's own unit test, and more importantly, each of the copy </div><div>pastes can be tested in isolation from the others. You cannot claim the same about changing instruction </div>
<div>selection. It has far reaching implications that go beyond the scope of unit testing, maybe beyond</div><div>the of testing. </div><div><br></div><div>This would be one of those things that you can only "check" by running every program you can get through</div>
<div>the compiler. When you reach that point, the project is in danger. You cannot rely on testing to expose bugs.</div><div>Testing is simply the last safety net. You should rely on architecture, proper engineering and proper review.</div>
<div>So here is one meaning of "best compromise": don't do anything that makes you fully dependent on testing.</div><div><br></div><div>And then you mention the question of risk. Whose risk is this? It's definitely not yours. There is some of my</div>
<div>risk because I'm making the changes and I am ultimately responsible for the consequences, not the </div><div>reviewer. But there are also people using LLVM in their own projects.. what about their risk? This is the true</div>
<div>risk: breaking something for someone using LLVM in a custom solution just because we</div><div>decided to be cowboys. This is the real risk and it isn't ours.. as such we should not take it.</div><div><br></div><div>
Regards,</div><div>Mihai</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 15, 2013 at 10:16 AM, Tim Northover <span dir="ltr"><<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Mihai,<br>
<div class="im"><br>
> The effort is not the issue here. Do you really think it's work the risk?<br>
<br>
</div>I think that if we're not making changes to LLVM because of the risk<br>
then the project has very serious problems, beyond being able to<br>
assemble weird edge cases of ARM instructions.<br>
<br>
I've seen projects that get like that, and it's not fun.<br>
<div class="im"><br>
> Do you really think this is a better and neater solution?<br>
<br>
</div>I do. I think it would be a change worth making even without this work<br>
in the same area motivating it. In fact I'm seriously considering<br>
taking a stab at it myself this weekend.<br>
<div class="im"><br>
> If you do, I'll do it - but I must say that the original patch I<br>
> submitted is the solution that makes more<br>
> sense simply because it keeps changes local.<br>
<br>
</div>How about doing this incrementally, then? I think a key question your<br>
patch doesn't address is how disassembly will be supported. I've<br>
mentioned it before, but these two lines should not produce the same<br>
output (it'll come up in MC Hammer eventually):<br>
<br>
$ echo 0x04,0x00,0x90,0xe2 | bin/llvm-mc -disassemble -arch=arm<br>
    adds r0, r0, #4<br>
$ echo 0x01,0x0f,0x90,0xe2 | bin/llvm-mc -disassemble -arch=arm<br>
    adds r0, r0, #4<br>
<br>
I think that the best solution to that problem would involve an<br>
instruction with essentially the form I'm advocating (modulo edge<br>
details like bit-packed imm or two imms and so on). The existing<br>
instructions (ADDri etc) would probably become isCodeGenOnly instead<br>
of the new ones (ADDrii etc) being isAsmParserOnly. So why don't we<br>
plan this in three phases:<br>
<br>
1. Commit your patch (or something similar).<br>
2. Add disassembly support to your new instructions, switch<br>
isAsmParserOnly to isCodeGenOnly on the old ones.<br>
3. Go through each use in CodeGen, adapting them to use the new<br>
instruction and remove the legacy ones. We can be sure all uses have<br>
been fixed if we remove the instructions entirely; attempted use will<br>
fail at compile-time.<br>
<br>
Even the names actually work out nicely. I don't think ADDrii is<br>
significantly worse than ADDri for those instructions, particularly as<br>
it would remind people who BuildMI them in future that they need to be<br>
careful.<br>
<br>
Cheers.<br>
<span class="HOEnZb"><font color="#888888"><br>
Tim.<br>
</font></span></blockquote></div><br></div></div>