This is an archive of the discontinued LLVM Phabricator instance.

Differential D114778

[XCOFF][FastISel] make fast isel can lower general intrinsics
ClosedPublic

Authored by shchenz on Nov 30 2021, 12:55 AM.

Details

Reviewers
qiucf
jsji
nemanjai
Jake-Egan
Group Reviewers
Restricted Project
Commits
rG2c46ca96e242: [PowerPC] fast isel can lower intrinsics call on AIX.
Summary

This patch is to make fast isel for 64bit XCOFF have the ability to select general intrinsics which will not be generated as function entry and function descriptor in the final assembly.

This hides some issues in SelectBasicBlock() -> Scheduler->EmitSchedule() for a single debug related intrinsic call, for example DBG_LABEL in D114685 and DBG_VALUE in D114686.

For example, for DBG_VALUE in patch D114686, the DBG_VALUE will be wrongly generated at the end of the function instead of the front like the IR shows, thus later DWARF info generation will miss generating some DWARF info(arguments/local variables) for the inlined subroutine.

We can fix this issue in EmitSchedule of course, but I think for our XCOFF case, the most sensible way is to let fast ISEL can lower these intrinsics calls so that we don't need to use DAG isel for these not well handled debug intrinsic calls in SelectBasicBlock() -> Scheduler->EmitSchedule().

Diff Detail

Event Timeline

shchenz created this revision.Nov 30 2021, 12:55 AM
Herald added a subscriber: hiraditya. · View Herald TranscriptNov 30 2021, 12:55 AM
shchenz requested review of this revision.Nov 30 2021, 12:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2021, 12:55 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
shchenz mentioned this in D114685: [AIX] XFAIL test missing DBG_LABEL.Nov 30 2021, 12:55 AM
shchenz mentioned this in D114686: [AIX] XFAIL test missing inlined argument/local variables for the inlined subroutine.
shchenz edited the summary of this revision. (Show Details)
shchenz updated this revision to Diff 390610.Nov 30 2021, 1:02 AM
qiucf added inline comments.Nov 30 2021, 1:16 AM
llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1784
  • In this case, no extra instruction will be really emitted from this intrinsic, but what if it's an intrinsic producing a real function call?
  • Is it necessary to put the check here, not in PPCFastISel::fastSelectInstruction?
llvm/test/CodeGen/PowerPC/debug-label-fast-isel.ll
2

nit: remove top empty line and use llc < %s -mtriple ... | FileCheck ...

Harbormaster completed remote builds in B136624: Diff 390610.Nov 30 2021, 1:43 AM
nemanjai added inline comments.Nov 30 2021, 3:14 AM
llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1784
  • In this case, no extra instruction will be really emitted from this intrinsic, but what if it's an intrinsic producing a real function call?

I don't think this is an issue since FastISel::selectIntrinsicCall() only handles a fixed list of intrinsics and the rest are handled in FastISel::fastLowerIntrinsicCall() which we don't override.

  • Is it necessary to put the check here, not in PPCFastISel::fastSelectInstruction?

+1
The fact that we have the same check in both places seems odd and problematic. And the fact that we are now diverging with those two checks is even stranger.

shchenz updated this revision to Diff 390869.Nov 30 2021, 5:13 PM
shchenz marked 3 inline comments as done.

address comments

Herald added a subscriber: kbarton. · View Herald TranscriptNov 30 2021, 5:13 PM
shchenz added inline comments.Nov 30 2021, 5:13 PM
llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1784

I deleted the redundant logic in PPCFastISel::fastSelectInstruction for Instruction::Call. It is exactly the same as the one in the target-independent one. And PowerPC will also use the target-independent ISel.

Harbormaster completed remote builds in B136810: Diff 390869.Nov 30 2021, 6:14 PM
shchenz added a comment.Dec 5 2021, 5:52 PM

gentle ping

shchenz added a comment.Dec 12 2021, 6:29 PM

gentle ping

shchenz added a comment.Dec 21 2021, 12:53 AM

gentle ping

shchenz added a comment.Jan 6 2022, 5:22 PM

gentle ping

craig.topper added a subscriber: craig.topper.Jan 6 2022, 10:18 PM
craig.topper added inline comments.
llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1784

Use isa instead of dyn_cast.

shchenz updated this revision to Diff 398049.Jan 6 2022, 10:32 PM

comment address

qiucf added a comment.Jan 6 2022, 10:36 PM

nit: Rename debug-label-fast-isel.ll to debug-label-fast-isel-xcoff.ll?

shchenz added a comment.Jan 6 2022, 10:42 PM
In D114778#3226718, @qiucf wrote:

nit: Rename debug-label-fast-isel.ll to debug-label-fast-isel-xcoff.ll?

Is this renaming from a public guideline? To me, it should be easy to know that this file is AIX only after checking the file.

qiucf accepted this revision as: qiucf.Jan 6 2022, 11:04 PM

LGTM.

In D114778#3226719, @shchenz wrote:

Is this renaming from a public guideline? To me, it should be easy to know that this file is AIX only after checking the file.

Not necessary.

This revision is now accepted and ready to land.Jan 6 2022, 11:04 PM
Harbormaster completed remote builds in B142016: Diff 398049.Jan 6 2022, 11:13 PM
Closed by commit rG2c46ca96e242: [PowerPC] fast isel can lower intrinsics call on AIX. (authored by shchenz). · Explain WhyJan 9 2022, 6:32 PM
This revision was automatically updated to reflect the committed changes.
shchenz added a commit: rG2c46ca96e242: [PowerPC] fast isel can lower intrinsics call on AIX..

Revision Contents

PathSize
llvm/
lib/
CodeGen/
SelectionDAG/
5 lines
test/
CodeGen/
PowerPC/
56 lines

Diff 390604

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp

Show First 20 Lines • Show All 1,769 Lines • ▼ Show 20 Lines case Instruction::Alloca:
// FunctionLowering has the static-sized case covered. // FunctionLowering has the static-sized case covered.
if (FuncInfo.StaticAllocaMap.count(cast<AllocaInst>(I))) if (FuncInfo.StaticAllocaMap.count(cast<AllocaInst>(I)))
return true; return true;
// Dynamic-sized alloca is not handled yet. // Dynamic-sized alloca is not handled yet.
return false; return false;
case Instruction::Call: case Instruction::Call:
// On AIX, call lowering uses the DAG-ISEL path currently so that the // On AIX, normal call lowering uses the DAG-ISEL path currently so that the
// callee of the direct function call instruction will be mapped to the // callee of the direct function call instruction will be mapped to the
// symbol for the function's entry point, which is distinct from the // symbol for the function's entry point, which is distinct from the
// function descriptor symbol. The latter is the symbol whose XCOFF symbol // function descriptor symbol. The latter is the symbol whose XCOFF symbol
// name is the C-linkage name of the source level function. // name is the C-linkage name of the source level function.
if (TM.getTargetTriple().isOSAIX()) // But fast isel still have the ability to do selection for intrinsics.
if (TM.getTargetTriple().isOSAIX() && !dyn_cast<IntrinsicInst>(I))
qiucfUnsubmitted
Done
ReplyInline Actions
  • In this case, no extra instruction will be really emitted from this intrinsic, but what if it's an intrinsic producing a real function call?
  • Is it necessary to put the check here, not in PPCFastISel::fastSelectInstruction?
qiucf: - In this case, no extra instruction will be really emitted from this intrinsic, but what if…
nemanjaiUnsubmitted
Done
ReplyInline Actions
  • In this case, no extra instruction will be really emitted from this intrinsic, but what if it's an intrinsic producing a real function call?

I don't think this is an issue since FastISel::selectIntrinsicCall() only handles a fixed list of intrinsics and the rest are handled in FastISel::fastLowerIntrinsicCall() which we don't override.

  • Is it necessary to put the check here, not in PPCFastISel::fastSelectInstruction?

+1
The fact that we have the same check in both places seems odd and problematic. And the fact that we are now diverging with those two checks is even stranger.

nemanjai: > - In this case, no extra instruction will be really emitted from this intrinsic, but what if…
shchenzAuthorUnsubmitted
Done
ReplyInline Actions

I deleted the redundant logic in PPCFastISel::fastSelectInstruction for Instruction::Call. It is exactly the same as the one in the target-independent one. And PowerPC will also use the target-independent ISel.

shchenz: I deleted the redundant logic in `PPCFastISel::fastSelectInstruction` for `Instruction::Call`.
craig.topperUnsubmitted
Not Done
ReplyInline Actions

Use isa instead of dyn_cast.

craig.topper: Use isa instead of dyn_cast.
return false; return false;
return selectCall(I); return selectCall(I);
case Instruction::BitCast: case Instruction::BitCast:
return selectBitCast(I); return selectBitCast(I);
case Instruction::FPToSI: case Instruction::FPToSI:
return selectCast(I, ISD::FP_TO_SINT); return selectCast(I, ISD::FP_TO_SINT);
▲ Show 20 Lines • Show All 578 Lines • Show Last 20 Lines

llvm/test/CodeGen/PowerPC/debug-label-fast-isel.ll

  • This file was added.
; RUN: llc %s -mtriple powerpc64-ibm-aix-xcoff -o - | FileCheck %s --check-prefix=CHECKASM
qiucfUnsubmitted
Done
ReplyInline Actions

nit: remove top empty line and use llc < %s -mtriple ... | FileCheck ...

qiucf: nit: remove top empty line and use `llc < %s -mtriple ... | FileCheck ...`
; This is a case copied from test/DebugInfo/Generic/debug-label-mi.ll. This test
; is to explicited that fast isel for XCOFF works as expected for debug related
; intrinsics.
; CHECKASM: DEBUG_LABEL: foo:top
; CHECKASM: DEBUG_LABEL: foo:done
source_filename = "debug-label-mi.c"
; Function Attrs: noinline nounwind optnone
define i32 @foo(i32 signext %a, i32 signext %b) #0 !dbg !4 {
entry:
%a.addr = alloca i32, align 4
%b.addr = alloca i32, align 4
%sum = alloca i32, align 4
store i32 %a, i32* %a.addr, align 4
store i32 %b, i32* %b.addr, align 4
br label %top
top: ; preds = %entry
call void @llvm.dbg.label(metadata !8), !dbg !9
%0 = load i32, i32* %a.addr, align 4
%1 = load i32, i32* %b.addr, align 4
%add = add nsw i32 %0, %1
store i32 %add, i32* %sum, align 4
br label %done
done: ; preds = %top
call void @llvm.dbg.label(metadata !10), !dbg !11
%2 = load i32, i32* %sum, align 4
ret i32 %2
}
; Function Attrs: nounwind readnone speculatable
declare void @llvm.dbg.label(metadata)
attributes #0 = { noinline nounwind optnone uwtable }
!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3}
!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: false, emissionKind: FullDebug, enums: !2)
!1 = !DIFile(filename: "debug-label-mi.c", directory: "./")
!2 = !{}
!3 = !{i32 2, !"Debug Info Version", i32 3}
!4 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !5, isLocal: false, isDefinition: true, scopeLine: 2, isOptimized: false, unit: !0, retainedNodes: !2)
!5 = !DISubroutineType(types: !6)
!6 = !{!7, !7, !7}
!7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!8 = !DILabel(scope: !4, name: "top", file: !1, line: 4)
!9 = !DILocation(line: 4, column: 1, scope: !4)
!10 = !DILabel(scope: !4, name: "done", file: !1, line: 7)
!11 = !DILocation(line: 7, column: 1, scope: !4)