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

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added a comment.

I am not really sure if this is correct way to fix PR42736. I looked in td files and found a few inconsistencies, again I might have misinterpreted something.

MipsHi(the sd node) seems to have multiple interpretations if we add new patterns

There is MipsHiLoRelocs multiclass in MipsInstrInfo.td

  multiclass MipsHiLoRelocs<Instruction Lui, Instruction Addiu,
                            Register ZeroReg, RegisterOperand GPROpnd> {
    def : MipsPat<(MipsHi tglobaladdr:$in), (Lui tglobaladdr:$in)>;
  ...

and in Mips64InstrInfo.td

  defm : MipsHiLoRelocs<LUi64, DADDiu, ZERO_64, GPR64Opnd>, ISA_MIPS3, GPR_64,
         SYM_32;

equivalent to

  def : MipsPat<(MipsHi tglobaladdr:$in), (LUi64 tglobaladdr:$in)>;

disagrees with the new

  def : MipsPat<(shl (MipsHi (i64 tglobaladdr:$in)), (i32 16)),
                (LUi64 tglobaladdr:$in)>, ISA_MIPS3, GPR_64;

In td file MipsHi is interpreted like: "16 bit imm(bits 31-16 of global addr in our case) is placed into bits 31-16"
while in getAddrNonPICSym64 it is: "16 bit imm(bits 31-16 of global addr in our case) is placed into low 16 bits of result and shift for 16 is required?"

If we use existing MipsISD::Hi interpretation from td file getAddrNonPICSym64 could be modified like this:

         SDValue HigherPart =
             DAG.getNode(ISD::ADD, DL, Ty, Highest,
                         DAG.getNode(MipsISD::Higher, DL, Ty, Higher));
  -      SDValue Cst = DAG.getConstant(16, DL, MVT::i32);
  +      SDValue Cst = DAG.getConstant(32, DL, MVT::i32);
         SDValue Shift = DAG.getNode(ISD::SHL, DL, Ty, HigherPart, Cst);
         SDValue Add = DAG.getNode(ISD::ADD, DL, Ty, Shift,
                                   DAG.getNode(MipsISD::Hi, DL, Ty, Hi));
  -      SDValue Shift2 = DAG.getNode(ISD::SHL, DL, Ty, Add, Cst);
   
  -      return DAG.getNode(ISD::ADD, DL, Ty, Shift2,
  +      return DAG.getNode(ISD::ADD, DL, Ty, Add,
                            DAG.getNode(MipsISD::Lo, DL, Ty, Lo));

Also MipsISD::Highest and MipsISD::Higher do not seem consistent with their names,
MipsISD::Highest places 16 bit imm(bits 48-63 of global addr in our case) into bits 31-16

MipsISD::Highest is selected like MipsISD::Hi (lui), while 
MipsISD::Higher is selected like MipsISD::Lo (addiu)

Replacing MipsISD::Highest and MipsISD::Higher with MipsISD::Hi and MipsISD::Lo in getAddrNonPICSym64, results in same instructions being selected.
Now, I might be missing something here since I am not that familiar with llvm options and ways that global address and similar should be transformed into instructions, 
but SDNodes

  def MipsHigher : SDNode<"MipsISD::Higher", SDTIntUnaryOp>;
  def MipsHighest : SDNode<"MipsISD::Highest", SDTIntUnaryOp>;

in MipsInstrInfo.td, and patterns they define seem to be duplicates of

  def MipsHi    : SDNode<"MipsISD::Hi", SDTIntUnaryOp>;
  def MipsLo    : SDNode<"MipsISD::Lo", SDTIntUnaryOp>;

and its patterns. Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66228