[CodeGen] Fixing inconsistent ABI mangling of vlaues in SelectionDAGBuilder

[pratlucas]

Summary

SelectionDAGBuilder was inconsistently mangling values based on ABI
Calling Conventions when getting them through copyFromRegs in
SelectionDAGBuilder, causing duplicate value type convertions for
function arguments. The checking for the mangling requirement was based
on the value's originating instruction and was performed outside of, and
inspite of, the regular Calling Convention Lowering.

The issue could be observed in a scenario such as:

%arg1 = load half, half* %const, align 2
%arg2 = call fastcc half @someFunc()
call fastcc void @otherFunc(half %arg1, half %arg2)
; Here, %arg2 was incorrectly mangled twice, as the CallConv data from
; the call to @someFunc() was taken into consideration for the check
; when getting the value for processing the call to @otherFunc(...),
; after the proper convertion had taken place when lowering the return
; value of the first call.

This patch fixes the issue by disregarding the Calling Convention
information for such copyFromRegs, making sure the ABI mangling is
properly contanined in the Calling Convention Lowering.

This fixes Bugzilla #47454.

[efriedma]

If I'm following this correctly, with this change, getABIRegCopyCC has one remaining caller, and that call always returns None. So getABIRegCopyCC is useless, and the CallConv member of RegsForValue is always None? It seems weird that all this code got built up around a useless operation, but I guess that sort of thing can happen.

@arsenm Do you remember why you added getABIRegCopyCC in the first place?

llvm/test/CodeGen/ARM/pr47454.ll

6

Please remove the definitions of getConstant and isEqual if they aren't relevant.

[arsenm]

In D87844#2279854, @efriedma wrote:

If I'm following this correctly, with this change, getABIRegCopyCC has one remaining caller, and that call always returns None. So getABIRegCopyCC is useless, and the CallConv member of RegsForValue is always None? It seems weird that all this code got built up around a useless operation, but I guess that sort of thing can happen.

@arsenm Do you remember why you added getABIRegCopyCC in the first place?

It's because the value types used differ if the copy is in an ABI context, or just to get copyable values. e.g. a phi is not an ABI context, but getRegisterTypeForCallingConv/getNumRegistersForCallingConv may be different from the legal register types used for the value

[pratlucas]

This seemed weird to me as well, specially as removing it had no impact on any of the regression tests.

From what I could check, though, the ABI mangling from the legal register types is already being performed by getCopyFromParts when collecting the return value inside SelectionDAGBuilder::LowerCallTo.
With that, when getting the return value afterwards, the ABI mangling was applied twice.

In the example from the commit message, what happened was the following:

%arg2 = call fastcc half @someFunc()                             ; Here, the return value of @sumeFunc went through ABI mangling properly inside SelectionDAGBuilder::LowerCallTo
call fastcc void @otherFunc(half %arg1, half %arg2)              ; When getting the value for the lowering of the %arg2 actual argument, the ABI mangling would happen again over the same value in `RegsForValue::getCopyFromRegs`

I've also noted that the calling convention argument is only being used for RegsForValue::getCopyToRegs in SelectionDAGBuilder::LowerStatepoint. All its other usages set the calling convention to None.

This inconsistency in the ABI mangling apparently caused no harm before, but after the introduction of the target-dependent splitting/joining of value parts by D75169 a difference in the final results can be observed.

[pratlucas]

Cleaning-up test and removing the getABIRegCopyCC function.

[efriedma]

LGTM

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

 // optimization, but could be lowered to improve compile time. Any ld-ld-st-st
 // sequence over this should have been converted to llvm.memcpy by the
 // frontend. It is easy to induce this behavior with .ll code such as:
 // %buffer = alloca [4096 x i8]
 // %data = load [4096 x i8]* %argPtr
 // store [4096 x i8] %data, [4096 x i8]* %buffer
 static const unsigned MaxParallelChains = 64;
 
-// Return the calling convention if the Value passed requires ABI mangling as it
-// is a parameter to a function or a return value from a function which is not
-// an intrinsic.
-static Optional<CallingConv::ID> getABIRegCopyCC(const Value *V) {
-  if (auto *R = dyn_cast<ReturnInst>(V))
-    return R->getParent()->getParent()->getCallingConv();
-
-  if (auto *CI = dyn_cast<CallInst>(V)) {
-    const bool IsInlineAsm = CI->isInlineAsm();
-    const bool IsIndirectFunctionCall =
-        !IsInlineAsm && !CI->getCalledFunction();
-
-    // It is possible that the call instruction is an inline asm statement or an
-    // indirect function call in which case the return value of
-    // getCalledFunction() would be nullptr.
-    const bool IsInstrinsicCall =
-        !IsInlineAsm && !IsIndirectFunctionCall &&
-        CI->getCalledFunction()->getIntrinsicID() != Intrinsic::not_intrinsic;
-
-    if (!IsInlineAsm && !IsInstrinsicCall)
-      return CI->getCallingConv();
-  }
-
-  return None;
-}
-
 static SDValue getCopyFromPartsVector(SelectionDAG &DAG, const SDLoc &DL,
                                       const SDValue *Parts, unsigned NumParts,
                                       MVT PartVT, EVT ValueVT, const Value *V,
                                       Optional<CallingConv::ID> CC);
 
 /// getCopyFromParts - Create a value that contains the specified legal parts
 /// combined into the value they represent.  If the parts combine to a type
 /// larger than ValueVT then AssertOp can be used to specify whether the extra
if (SI != FuncInfo.StaticAllocaMap.end())
                                TLI.getFrameIndexTy(DAG.getDataLayout()));
   }
 
   // If this is an instruction which fast-isel has deferred, select it now.
   if (const Instruction *Inst = dyn_cast<Instruction>(V)) {
     unsigned InReg = FuncInfo.InitializeRegForValue(Inst);
 
     RegsForValue RFV(*DAG.getContext(), TLI, DAG.getDataLayout(), InReg,
-                     Inst->getType(), getABIRegCopyCC(V));
+                     Inst->getType(), None);
     SDValue Chain = DAG.getEntryNode();
     return RFV.getCopyFromRegs(DAG, FuncInfo, getCurSDLoc(), Chain, nullptr, V);
   }
 
   if (const MetadataAsValue *MD = dyn_cast<MetadataAsValue>(V)) {
     return DAG.getMDNode(cast<MDNode>(MD->getMetadata()));
   }
   llvm_unreachable("Can't get register for value!");
if (!Op) {
     };
 
     // Check if ValueMap has reg number.
     DenseMap<const Value *, Register>::const_iterator
       VMI = FuncInfo.ValueMap.find(V);
     if (VMI != FuncInfo.ValueMap.end()) {
       const auto &TLI = DAG.getTargetLoweringInfo();
       RegsForValue RFV(V->getContext(), TLI, DAG.getDataLayout(), VMI->second,
-                       V->getType(), getABIRegCopyCC(V));
+                       V->getType(), None);
       if (RFV.occupiesMultipleRegs()) {
         splitMultiRegDbgValue(RFV.getRegsAndSizes());
         return true;
       }
 
       Op = MachineOperand::CreateReg(VMI->second, false);
       IsIndirect = IsDbgDeclare;
     } else if (ArgRegsAndSizes.size() > 1) {

llvm/test/CodeGen/ARM/pr47454.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=armv8-unknown-linux-unknown -mattr=-fp16 -O0 < %s | FileCheck %s
+
+declare fastcc half @getConstant()
+
+declare fastcc i1 @isEqual(half %0, half %1)
+
+define internal fastcc void @main() {
+; CHECK-LABEL: main:
+; CHECK:       @ %bb.0: @ %Entry
+; CHECK-NEXT:    push {r11, lr}
+; CHECK-NEXT:    mov r11, sp
+; CHECK-NEXT:    sub sp, sp, #16
+; CHECK-NEXT:    mov r0, #31744
+; CHECK-NEXT:    strh r0, [r11, #-2]
+; CHECK-NEXT:    ldrh r0, [r11, #-2]
+; CHECK-NEXT:    bl __gnu_h2f_ieee
+; CHECK-NEXT:    vmov s0, r0
+; CHECK-NEXT:    vstr s0, [sp, #8] @ 4-byte Spill
+; CHECK-NEXT:    bl getConstant
+; CHECK-NEXT:    vmov r0, s0
+; CHECK-NEXT:    bl __gnu_h2f_ieee
+; CHECK-NEXT:    vmov s0, r0
+; CHECK-NEXT:    vmov r0, s0
+; CHECK-NEXT:    bl __gnu_f2h_ieee
+; CHECK-NEXT:    vldr s0, [sp, #8] @ 4-byte Reload
+; CHECK-NEXT:    vmov r1, s0
+; CHECK-NEXT:    str r0, [sp, #4] @ 4-byte Spill
+; CHECK-NEXT:    mov r0, r1
+; CHECK-NEXT:    bl __gnu_f2h_ieee
+; CHECK-NEXT:    uxth r0, r0
+; CHECK-NEXT:    vmov s0, r0
+; CHECK-NEXT:    ldr r0, [sp, #4] @ 4-byte Reload
+; CHECK-NEXT:    uxth r1, r0
+; CHECK-NEXT:    vmov s1, r1
+; CHECK-NEXT:    bl isEqual
+; CHECK-NEXT:    mov sp, r11
+; CHECK-NEXT:    pop {r11, pc}
+Entry:
+    ; First arg directly from constant
+    %const = alloca half, align 2
+    store half 0xH7C00, half* %const, align 2
+    %arg1 = load half, half* %const, align 2
+    ; Second arg from fucntion return
+    %arg2 = call fastcc half @getConstant()
+    ; Arguments should have equivalent mangling
+    %result = call fastcc i1 @isEqual(half %arg1, half %arg2)
+    ret void
+}