[PATCH] D66228: [mips] Fix 64-bit address loading in case of applying 32-bit mask to the result

Wed Aug 21 10:49:08 PDT 2019

sdardis added a comment.

In D66228#1635859 <https://reviews.llvm.org/D66228#1635859>, @atanasyan wrote:

> Thanks for review. Could you clarify some points in your comments?
>
> In D66228#1632060 <https://reviews.llvm.org/D66228#1632060>, @sdardis wrote:
>
> > The correct approach I believe is to fix the SYM_32 bug, then provide the patterns for cases of where intermediate/end nodes such as MipsHi / MipsLo can appear without an add appearing as their parent node.
>
>
>
>
> 1. If we do not change the `getAddrNonPICSym64` function, at some point (before lowering) we anyway(?) get the following chain of commands `(add (shl (%hi(sym), 16), %lo(%sym))`. How can we lower it to a correct set of instructions, if we do not have a pattern with the `shl`? Please correct we if I'm wrong.

Match the MipsHi to daddiu zero, MipsHi. The shift will move the Hi value to the correct place, this is correct behaviour. If you look at the stanza of patterns for Higher, you'll notice they're duplicated, both (MipsHigher (i64 symboltype:%sum)) and (add $highest (MipsHigher (i64 symboltype:%sum))). Providing the first set of patterns for (MipsHi (i64 symbol)) will allow DAGISel to pick the correct instructions. We can''t use LUi64 in this case as it would sign extend the symbol value into the upper 32 bits.

> 2. What do you mean by //"patterns for cases of where intermediate/end nodes such as MipsHi / MipsLo can appear without an add appearing as their parent node"//? Do you mean a pattern like `add zero, MipsHi(tglobaladdr)`, or just `MipsHi(tglobaladdr)` or something else?

We need to ensure that (add Hi$, MipsHi/Higher(tglobaladdr)) can be selected for the chain of nodes created by getAddrNonPICSym64. If that series of nodes is modified we could have a case where the .td patterns don't match what getAddrNonPICSym64 produce, and DAGISel sees (shl (MipsHi (tglobaladdr), 16). We need two sets of patterns, one for (add (MipsHi ...)) and (MipsHi ..).

Also, without fixing the sym32 bug, this patch won't produce the correct result.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66228/new/

https://reviews.llvm.org/D66228