Page MenuHomePhabricator

[CodeGen] Fixing inconsistent ABI mangling of vlaues in SelectionDAGBuilder
ClosedPublic

Authored by pratlucas on Sep 17 2020, 10:33 AM.

Details

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.

Diff Detail

Event Timeline

pratlucas created this revision.Sep 17 2020, 10:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2020, 10:33 AM
pratlucas requested review of this revision.Sep 17 2020, 10:33 AM

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.

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

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 updated this revision to Diff 292788.Sep 18 2020, 7:51 AM

Cleaning-up test and removing the getABIRegCopyCC function.

pratlucas marked an inline comment as done.Sep 18 2020, 7:53 AM
This revision is now accepted and ready to land.Sep 18 2020, 11:08 AM