[PATCH] ARM: use new modified-imm instructions for all MC-level purposes.

[Kristof Beyls]

Hi Tim,

 

Wouldn't it be possible to get rid of the "ri" MCInst versions quite easily,
by adding a switch statement in
llvm::LowerARMMachineInstrToMCInst?
If possible, the switch statement would translate MachineInstr's with one
immediate operand to 
MCInst'st with 2 operands (the immediate and the rotation). It seems to me
that such a switch statement 
would only need about 20 case statements.
While adding such a switch isn't nice, it would mean that the duplication of
MCInsts could be removed
immediately, which in my opinion is uglier than having 1 switch statement
with about 20 cases in
LowerARMMachineInstrToMCInst.

 

I have to say I'm not convinced (yet?) that also making the encoding of
modified immediates more 
explicit in MachineInstr's would be very nice. Without having looked into it
much, it seems to me that a 
single switch statement with 20 cases is less ugly than adding lots of
immediate+rotation values in places 
where MachineInstr's get created, especially because I'm assuming that at
the point where you're
creating MachineInstr's, you're actually not interested in how the immediate
gets encoded in detail?

 

With that switch statement, in the MC layer, instructions with modified
immediates would always be
represented in the *rii form, i.e. as an immediate+a rotation, removing
duplication. In MachineInstr's 
immediates would always be represented as a single integer, as is currently
the case.

 

I'm sure I'm missing quite a lot of the details here, but would this
approach seem reasonable to you?

 

Thanks,

 

Kristof

 

From: llvm-commits-bounces at cs.uiuc.edu
[mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Renato Golin
Sent: 27 August 2013 13:41
To: reviews+D1432+public+43eb1c3b208b4e68 at llvm-reviews.chandlerc.com
Cc: LLVM Commits
Subject: Re: [PATCH] ARM: use new modified-imm instructions for all MC-level
purposes.

 

Hi Tim,

 

Overall, I like this change and I agree it's the way forward. I had some
nitpick comments in phabricator, but I'm happy with the patch as it is. Feel
free to ignore whatever doesn't make sense.

 

cheers,

--renato

 

On 17 August 2013 16:33, Tim Northover <t.p.northover at gmail.com> wrote:

  Noticed a couple of tidy ups just after posting. Isn't it always the way?
    + use "ARMModifiedImm" instead of "ModifiedImm" in printing. There's a
Thumb one too.
    + An 80-col thing, that I'd even annotated "FIXME".

Hi grosbach,

http://llvm-reviews.chandlerc.com/D1432

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D1432?vs=3545
<http://llvm-reviews.chandlerc.com/D1432?vs=3545&id=3547#toc> &id=3547#toc


Files:
  lib/Target/ARM/ARMCodeEmitter.cpp
  lib/Target/ARM/ARMInstrInfo.td
  lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  lib/Target/ARM/Disassembler/ARMDisassembler.cpp
  lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp
  lib/Target/ARM/InstPrinter/ARMInstPrinter.h
  lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
  test/MC/ARM/basic-arm-instructions.s
  test/MC/ARM/diagnostics.s

  test/MC/Disassembler/ARM/arm-tests.txt

_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130827/4d5eb7ec/attachment.html>