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

[Simon Dardis via Phabricator via llvm-commits]

sdardis requested changes to this revision.
sdardis added a comment.
This revision now requires changes to proceed.

I took a second look, and I believe this patch is the incorrect solution. The bug actually lies in the implementation of PredicateControl and SYM_32/SYM_64, @Petar.Avramovic nearly spotted it.

Those two adjectives add a predicate which checks if the sym32 feature is enabled or not[1]. However, as they are not appended to the list of predicates for a pattern--as SYMPredicates is not in the definition of PredicateControl.

Without that adjective taking effect there are two sets of patterns for a MipsHi node depending on it's parent. If the parent node is a register and an add--as expected--the instruction selection machinery produces an DADDiu with the relevant operand and relocation. //If// it is not an add, i.e. the case where the known values of the upper 32 bits are zero which would allow DAGCombiner to elide the addition of the upper components and the addition of the upper components to the MipsHi node as they are all zero, then the instruction selection machinery can't maximally munch to the graph like in the (add (MipsHi ...) ..) case and picks the transformation of MipsHi -> LUi64, when that node is the child of a shift //because// the SYM_32 patterns provide for that result at a lower complexity.

Those patterns should not be selectable, leading to an instruction selection failure. Since they are selectable, the result of an LUi64 gets shifted into MipsHigher's space then has MipsLo added to it.

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] There's a bug there too! it should be hasSym32() rather than HasSym32().



================
Comment at: llvm/lib/Target/Mips/Mips64InstrInfo.td:687
+  def : MipsPat<(shl (MipsHi (i64 tglobaladdr:$in)), (i32 16)),
+                (LUi64 tglobaladdr:$in)>, ISA_MIPS3, GPR_64;
+  def : MipsPat<(shl (MipsHi (i64 tblockaddress:$in)), (i32 16)),
----------------
sdardis wrote:
> This addition and below needs SYM_64 I believe.
I don't believe this comment is relevant now.


================
Comment at: llvm/test/CodeGen/Mips/global-address-with-mask.ll:1
+; RUN: llc -mtriple=mips64-linux-gnuabi64 \
+; RUN:     -relocation-model=pic < %s | FileCheck %s -check-prefix=PIC
----------------
Call this file pr42736.ll


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66228