[PowerPC] Don't reuse the address of an illegal typed load for int_to_fp.

[sfertile]

Summary

When the operand to an (s/u)int_to_fp node is an illegally typed load we cannot reuse the load address since we can't build a proper dependancy chain. The legalized loads will use a different chain output then the illegal load. If we reuse the load address then we will build a conversion node that uses the chain of the illegal load and operations which modify the memory address in the other dependancy chain can be scheduled before the floating point load which feeds the conversion.

[ZarkoCA]

llvm/test/CodeGen/PowerPC/cvt_i64_to_fp.ll

4

Out of curiousity, is there a reason for specifying freebsd vs linux?

7

nit: I don't think you need the #0 here.

[nemanjai]

The patch LGTM, but I really think the test case should change to more clearly convey the behaviour you desire to test.

llvm/test/CodeGen/PowerPC/cvt_i64_to_fp.ll

16

Am I misreading this? It looks like the test is actually testing for the undesired behaviour. Namely, we load the value from the parameter pointer (presumably increment it there), store it to stack - 8, then load it from stack - 8 into an FPR and convert.

Of course, this comment is tongue-in-cheek and I assume that the stores to the stack are before the increment and there are a pair of stores to the parameter pointer of the incremented value - but there is nothing in this test case to show that. Namely, this would equivalently match this wrong sequence:

lwz 5, 4(3)
addic 5, 5, 1
stw 5, -4(1)
stw 5, 4(3)
lwz 5, 0(3)
addze 5, 5
stw 5, -8(1)
stw 5, 0(3)
lfd 1, -8(1)
fcfid 1, 1

So my advice is to use the script to produce the checks as it is more maintainable and easy to prevent issues such as this. However, if you prefer not to use it, you should at least show the increment and the other stores to show that we are storing the incremented value to the parameter pointer and the non-incremented value to the stack.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

bool PPCTargetLowering::canReuseLoadAddress(SDValue Op, EVT MemVT,
 
   LoadSDNode *LD = dyn_cast<LoadSDNode>(Op);
   if (!LD || LD->getExtensionType() != ET || LD->isVolatile() ||
       LD->isNonTemporal())
     return false;
   if (LD->getMemoryVT() != MemVT)
     return false;
 
+  // If the result of the load is an illegal type, then we can't build a
+  // valid chain for reuse since the legalised loads and token factor node that
+  // ties the legalised loads together uses a different output chain then the
+  // illegal load.
+  if (!isTypeLegal(LD->getValueType(0)))
+    return false;
+
   RLI.Ptr = LD->getBasePtr();
   if (LD->isIndexed() && !LD->getOffset().isUndef()) {
     assert(LD->getAddressingMode() == ISD::PRE_INC &&
            "Non-pre-inc AM on PPC?");
     RLI.Ptr = DAG.getNode(ISD::ADD, dl, RLI.Ptr.getValueType(), RLI.Ptr,
                           LD->getOffset());
   }
 

llvm/test/CodeGen/PowerPC/cvt_i64_to_fp.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN:  llc --verify-machineinstrs -mtriple powerpc-unknown-freebsd \
+; RUN:      -mcpu=pwr4 < %s | FileCheck %s
+
+define double @postinctodbl(i64* nocapture %llp) #0 {
+; CHECK-LABEL: postinctodbl:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    stwu 1, -16(1)
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    lwz 4, 4(3)
+; CHECK-NEXT:    stw 4, 12(1)
+; CHECK-NEXT:    addic 4, 4, 1
+; CHECK-NEXT:    lwz 5, 0(3)
+; CHECK-NEXT:    stw 5, 8(1)
+; CHECK-NEXT:    addze 5, 5
+; CHECK-NEXT:    lfd 0, 8(1)
+; CHECK-NEXT:    stw 5, 0(3)
+; CHECK-NEXT:    fcfid 1, 0
+; CHECK-NEXT:    stw 4, 4(3)
+; CHECK-NEXT:    addi 1, 1, 16
+; CHECK-NEXT:    blr
+entry:
+  %0 = load i64, i64* %llp, align 8
+  %inc = add nsw i64 %0, 1
+  store i64 %inc, i64* %llp, align 8
+  %conv = sitofp i64 %0 to double
+  ret double %conv
+}