[PATCH] D18802: Improve support for i386 and i486 CPUs.

[Ahmed Bougacha via llvm-commits]

ab added a comment.

In http://reviews.llvm.org/D18802#393757, @jyknight wrote:

> > For BSWAP: would it work to do the custom lowering at ISel time instead of using a pseudo? I think it's possible to even write a pattern.
>
>
> I don't know, is it? You'd need to be able to express the operation "rotate bottom 16 bits of this register by 8 bits, in place, leaving the upper 16 bits as they were". It's not a normal 16-bit operation, which would let you allocate 16-bit inputs/output virtual registers, because the contents of the upper bits are important.


For the pattern: in my mind it was simple enough that you could duplicate the intermediate GR32 and let MachineCSE clean it up;  but I tried writing it and it's actually pretty damn unwieldy:

  def : Pat<(bswap GR32:$src),
      (INSERT_SUBREG
       (ROR32ri
        (INSERT_SUBREG (i32 GR32:$src),
         (ROR16ri (EXTRACT_SUBREG (i32 GR32:$src), sub_16bit), 8),
         sub_16bit),
        16),
       (ROR16ri
        (EXTRACT_SUBREG
         (ROR32ri
          (INSERT_SUBREG (i32 GR32:$src),
           (ROR16ri (EXTRACT_SUBREG (i32 GR32:$src), sub_16bit), 8),
           sub_16bit),
          16),
         sub_16bit),
        8),
       sub_16bit)
      >;

However, I think anything you can do in pseudo expansion, you could still do in ISelDAGToDAG, no?

Ideally you'd do it in SDAG, but there you're right, I don't think you can shoehorn it into the type system (not unless having machine nodes or something).


================
Comment at: lib/Target/X86/X86.td:279
@@ -272,1 +278,3 @@
+def : Proc<"pentium-mmx",     [FeatureX87, FeatureSlowUAMem16, FeatureMMX, Feature586Insns]>;
+def : Proc<"i686",            [FeatureX87, FeatureSlowUAMem16, Feature586Insns]>;
 def : Proc<"pentiumpro",      [FeatureX87, FeatureSlowUAMem16, FeatureCMOV]>;
----------------
jyknight wrote:
> ab wrote:
> > Should these features be on the remaining models?  It'd probably be time to do ProcessorFeatures lists like SNB.
> I think it doesn't need to be, because I added it as implied by cmov, which everything else supports?
You're right, missed that, sorry!


http://reviews.llvm.org/D18802