[llvm-commits] [llvm] r57262 - /llvm/trunk/lib/Target/ARM/ARMInstrInfo.td

[Chris Lattner]

On Oct 8, 2008, at 1:18 PM, Jim Grosbach wrote:

> On Oct 7, 2008, at 7:46 PM, Chris Lattner wrote:
>>> +++ llvm/trunk/lib/Target/ARM/ARMInstrInfo.td Tue Oct  7 16:08:09  
>>> 2008
>>> @@ -567,7 +567,7 @@
>>> // B is "predicable" since it can be xformed into a Bcc.
>>> let isBarrier = 1 in {
>>>   let isPredicable = 1 in
>>> -    def B : AXI<0xA, (outs), (ins brtarget:$target), Branch, "b
>>> $target",
>>> +    def B : ABI<{0,1,0,1}, (outs), (ins brtarget:$target), Branch,
>>> "b $target",
>>>               [(br bb:$target)]>;
>>
>> I assume you're changing 0xA to 0,1,0,1 due to an endianness issue?
>> Why not just change the ABI/AXI patterns to do the bit swapping?  I'd
>> strongly prefer to have all the instructions (like B) have clear and
>> easy to understand encodings in them.  I'd rather push any  
>> complexity/
>> weirdness up into parent tblgen classes.
>
>
> Hi Chris,
>
> I absolutely agree, and that's something I'm going to look at fixing  
> in the near future. Right now, the assumption is that opcode values  
> are laid out such that the MSB is on the right. I don't much care  
> for that since the ARM documentation shows things w/ MSB on the  
> left. There's already a lot of confusion about which order the  
> opcode pattern should take in the the ARM .td files. There's quite a  
> few instructions that encode improperly due to that. I'm going to  
> try to get that straightened out and have everything be represented  
> as it looks in the ARM docs.
>
> That was outside the scope of what I wanted to accomplish in this  
> patch, though. For this, I just wanted to get a simple unconditional  
> branch encoding correctly so I can have some simple test code (a  
> Hanoi solver, in this case) running via the JIT.
>
> Short version, "I agree. Fixing that format confusion is next on the  
> agenda."

Ok, I guess my metapoint is that we should aim for the .td file to  
follow the manual (which I haven't looked at recently) as closely as  
possible.  handling it as a separate patch is fine by me :)

-Chris