[PATCH] D31676: [SelectionDAG] [ARM CodeGen] Fix chain information of LowerMUL

[James Molloy via Phabricator via llvm-commits]

jmolloy accepted this revision.
jmolloy added a comment.
This revision is now accepted and ready to land.

Hi,

It took me a while to grok this, so here's some ascii-art for the record :)

We start with the general case that we have a load whose value may be used by multiple other values:

    |


    LD
   / | \
  ... ...

Now we introduce a new load, LD', that *may* replace LD for some or all of LD's uses, but we don't know \
or do that transform at this time.

       +
     /  \
    LD   LD'
   / | \
  ... ...

Transferring value uses (SDValue(LD, 0)) can be done incrementally by optimization passes. But currently\
 we have two loads, one of which is unordered with respect to all other memory ops. There are two ways t\
o resolve this:

1. Create a TokenFactor to link the chains together;

We'd end up with something like:

    +
   / \
  LD  LD'
   \ /
    TF
   / | \
  .. .. ..

This will work. It does however confuse the optimizer into keeping both loads around (I think it can't d\
etermine that one of the loads can be rewritten in terms of the other:

  add     r3, r0, #16
  vld1.16 {d17}, [r3:64]
  vldr    d20, [r0, #16]

2. Remove one load and write it in terms of the other

This is what this patch is doing:

     +
     |
    LD'
     |
    EXT   <-- LD = LD' + EXT
   / | \
  .. .. ..

My concern with this, as it's planting an EXT, is whether the comment in SkipLoadExtensionForVMULL still\
applies:

  // We need to create a zextload/sextload. We cannot just create a load
  // followed by a zext/zext node because LowerMUL is also run during normal
  // operation legalization where we can't create illegal types.

I think that the planted EXT is fine, because it is extending always from a legal type to another legal \
type.

Therefore, LGTM.


Repository:
  rL LLVM

https://reviews.llvm.org/D31676