This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISelEmitter] handle operand without MVT/class
ClosedPublic

Authored by shchenz on Jan 8 2023, 8:24 PM.

Details

Summary

There are some patterns in td files without MVT/class set for some operands in target
pattern that are from the source pattern. This prevents GlobalISelEmitter from adding
them as a valid rule, because the target child operand is an unsupported kind operand.
For now, for a leaf child, only IntInit and DefInit are handled in GlobalISelEmitter.

This issue can be workaround by adding MVT/class to the patterns in the td files, like
the workarounds for patterns anyext and setcc in PPCInstrInfo.td in D140878.

To avoid adding the same workarounds for other patterns in td files, this patch tries to
handle the UnsetInit case in GlobalISelEmitter.

Adding the new handling allows us to remove the workarounds in the td files and also
generates many selection rules for PPC target.

Diff Detail

Event Timeline

shchenz created this revision.Jan 8 2023, 8:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2023, 8:24 PM
shchenz requested review of this revision.Jan 8 2023, 8:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2023, 8:24 PM

gentle ping.
We really want this to move forward on PPC. More patterns on PPC tds are without MVTs or register classes. Thank you!

arsenm added a subscriber: jrbyrnes.Mar 6 2023, 4:46 AM

Can you add a tablegen test for this?

llvm/test/CodeGen/AMDGPU/GlobalISel/fdiv.f16.ll
673–674

This is an improvement

1283

Improvement

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.g16.a16.dim.ll
34–35

This is a regression cc @jrbyrnes

llvm/utils/TableGen/GlobalISelEmitter.cpp
4647–4648

Where is the type check performed?

shchenz updated this revision to Diff 503316.Mar 8 2023, 4:29 AM
shchenz marked an inline comment as done.

add a new case GlobalISelEmitter-notypeoperand.ll

shchenz added inline comments.Mar 8 2023, 4:30 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4647–4648

Do you mean check the type for the output pattern's operands? I am not familiar here, do we need type check for the output pattern operands? And the input operands type check should already be handled in previous logic. See the new introduced rule in the new tblgen case GlobalISelEmitter-notypeoperand.ll:

constexpr static int64_t MatchTable0[] = {
  GIM_Try, /*On fail goto*//*Label 0*/ 74, // Rule ID 0 //
    GIM_CheckNumOperands, /*MI*/0, /*Expected*/2,
    GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_ANYEXT,
    // MIs[0] dst
    GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
    GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
    // MIs[0] in
    GIM_CheckType, /*MI*/0, /*Op*/1, /*Type*/GILLT_s8,
    // (anyext:{ *:[i32] } i8:{ *:[i8] }:$in)  =>  (SELECT_I4:{ *:[i32] } ?:{ *:[i8] }:$in, (LI:{ *:[i32] } 1:{ *:[i32] }), (LI:{ *:[i32] } 0:{ *:[i32] }))
    GIR_MakeTempReg, /*TempRegID*/0, /*TypeID*/GILLT_s32,
    GIR_MakeTempReg, /*TempRegID*/1, /*TypeID*/GILLT_s32,
    GIR_BuildMI, /*InsnID*/2, /*Opcode*/MyTarget::LI,
    GIR_AddTempRegister, /*InsnID*/2, /*TempRegID*/1, /*TempRegFlags*/RegState::Define,
    GIR_AddImm, /*InsnID*/2, /*Imm*/0,
    GIR_ConstrainSelectedInstOperands, /*InsnID*/2,
    GIR_BuildMI, /*InsnID*/1, /*Opcode*/MyTarget::LI,
    GIR_AddTempRegister, /*InsnID*/1, /*TempRegID*/0, /*TempRegFlags*/RegState::Define,
    GIR_AddImm, /*InsnID*/1, /*Imm*/1,
    GIR_ConstrainSelectedInstOperands, /*InsnID*/1,
    GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::SELECT_I4,
    GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
    GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // in
    GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/0,
    GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/1, /*TempRegFlags*/0,
    GIR_EraseFromParent, /*InsnID*/0,
    GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
    // GIR_Coverage, 0,
    GIR_Done,
  // Label 0: @74
  GIM_Reject,
  };
arsenm added inline comments.Mar 8 2023, 6:52 PM
llvm/test/TableGen/GlobalISelEmitter-notypeoperand.ll
1 ↗(On Diff #503316)

This should be a .td file, not .ll

7 ↗(On Diff #503316)

Should check the actual checks performed, not just there is a matcher

llvm/utils/TableGen/GlobalISelEmitter.cpp
4647–4648

I meant more where that gets emitted in the emitter code, not the output

shchenz updated this revision to Diff 504975.Mar 14 2023, 1:02 AM

fix the table-gen tests

shchenz marked an inline comment as done.Mar 14 2023, 1:12 AM
shchenz added inline comments.
llvm/utils/TableGen/GlobalISelEmitter.cpp
4647–4648

Hope I understand your comments right. The type check for the input pattern are added before the renders handling in GlobalISelEmitter::runOnPattern(). To be specific, type check are added in createAndImportSelDAGMatcher() where type checks are added to source pattern results and inputs.

Will this impact how this issue is handled?

Thank you!

arsenm accepted this revision.Mar 14 2023, 7:03 AM

LGTM

This revision is now accepted and ready to land.Mar 14 2023, 7:03 AM
shchenz marked an inline comment as done.Mar 14 2023, 11:13 PM

@arsenm Hi Matt, thanks for confirmation for the direction. I will continue to fix all the LIT failures and the crashes in the AMDGPU LIT cases.

Hi @arsenm , I created a github issue for the crashes this patch hits, would you please help to have a look? https://github.com/llvm/llvm-project/issues/61430. Thanks!

foad added a subscriber: foad.Mar 17 2023, 3:04 AM
shchenz updated this revision to Diff 512067.Apr 9 2023, 9:50 PM
shchenz retitled this revision from [GlobalISelEmitter][WIP] handle operand without MVT/class to [GlobalISelEmitter] handle operand without MVT/class.
shchenz edited the summary of this revision. (Show Details)

Change this to formal review

shchenz requested review of this revision.Apr 9 2023, 9:58 PM

Request formal review. I saw improvement and degradations for AMDGPU changes, but in general I think we should try to reuse the td file patterns as more as possible?

llvm/test/CodeGen/AArch64/arm64-vcvt.ll
405 ↗(On Diff #512067)

Before the change:

remark: <unknown>:0:0: cannot select: %1:gpr(s64) = G_INTRINSIC intrinsic(@llvm.aarch64.neon.fcvtzs), %2:gpr(s64) (in function: fcvtzs_1d_intrinsic)
warning: Instruction selection used fallback path for fcvtzs_1d_intrinsic

After the change:

	fcvtzs	x8, d0
	fmov	d0, x8
	ret

This should be expected as now global-isel are reusing more patterns defined in TD files. But here there is an opportunity to remove the fmov?

gentle ping

arsenm accepted this revision.Apr 18 2023, 6:43 PM
This revision is now accepted and ready to land.Apr 18 2023, 6:43 PM
This revision was landed with ongoing or failed builds.Apr 19 2023, 12:01 AM
This revision was automatically updated to reflect the committed changes.