This is an archive of the discontinued LLVM Phabricator instance.

Differential D142791

[GlobalISel] Don't combine instructions with unknown type in hoist_logic_op_with_same_opcode_hands.
ClosedPublic

Authored by dzhidzhoev on Jan 27 2023, 4:40 PM.

Details

Reviewers
paquette
arsenm
dsanders
aemerson
Commits
rG2a45d061ef81: [GlobalISel] Don't combine instructions with unknown type in…
Summary

This fixes failure for some LLVM IR.

Diff Detail

Event Timeline

dzhidzhoev created this revision.Jan 27 2023, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 4:40 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
dzhidzhoev requested review of this revision.Jan 27 2023, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 4:40 PM
Herald added subscribers: llvm-commits, wdng. · View Herald Transcript
paquette added inline comments.Jan 27 2023, 4:52 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2753

Do you need to check for validity for both XTy and YTy?

Harbormaster completed remote builds in B210498: Diff 492948.Jan 27 2023, 6:07 PM
dzhidzhoev updated this revision to Diff 492968.Jan 27 2023, 7:21 PM

Addressed @paquette comment.

Harbormaster completed remote builds in B210509: Diff 492968.Jan 27 2023, 8:18 PM
paquette accepted this revision.Jan 30 2023, 10:07 AM

LGTM

This revision is now accepted and ready to land.Jan 30 2023, 10:07 AM
Closed by commit rG2a45d061ef81: [GlobalISel] Don't combine instructions with unknown type in… (authored by dzhidzhoev). · Explain WhyJan 31 2023, 3:54 PM
This revision was automatically updated to reflect the committed changes.
dzhidzhoev added a commit: rG2a45d061ef81: [GlobalISel] Don't combine instructions with unknown type in….

Revision Contents

PathSize
llvm/
lib/
CodeGen/
GlobalISel/
2 lines
test/
CodeGen/
AArch64/
GlobalISel/
17 lines

Diff 492968

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

Show First 20 Lines • Show All 2,744 Lines • ▼ Show 20 Lines if (!LeftHandInst->getOperand(1).isReg() ||
return false; return false;
// Make sure the types match up, and if we're doing this post-legalization, // Make sure the types match up, and if we're doing this post-legalization,
// we end up with legal types. // we end up with legal types.
Register X = LeftHandInst->getOperand(1).getReg(); Register X = LeftHandInst->getOperand(1).getReg();
Register Y = RightHandInst->getOperand(1).getReg(); Register Y = RightHandInst->getOperand(1).getReg();
LLT XTy = MRI.getType(X); LLT XTy = MRI.getType(X);
LLT YTy = MRI.getType(Y); LLT YTy = MRI.getType(Y);
if (XTy != YTy) if (!XTy.isValid() || XTy != YTy)
paquetteUnsubmitted
Not Done
ReplyInline Actions
LLT YTy = MRI.getType(Y);
- if (!XTy.isValid() || !YTy.isValid() || XTy != YTy)
+ if (!XTy.isValid() || XTy != YTy)
return false;

Do you need to check for validity for both XTy and YTy?

paquette: Do you need to check for validity for both `XTy` and `YTy`?
return false; return false;
if (!isLegalOrBeforeLegalizer({LogicOpcode, {XTy, YTy}})) if (!isLegalOrBeforeLegalizer({LogicOpcode, {XTy, YTy}}))
return false; return false;
// Optional extra source register. // Optional extra source register.
Register ExtraHandOpSrcReg; Register ExtraHandOpSrcReg;
switch (HandOpcode) { switch (HandOpcode) {
default: default:
▲ Show 20 Lines • Show All 3,435 Lines • Show Last 20 Lines

llvm/test/CodeGen/AArch64/GlobalISel/postlegalizercombiner-hoist-same-hands.mir

Show First 20 Lines • Show All 52 Lines • ▼ Show 20 Lines bb.0:
%y_wide:_(s32) = COPY $w1 %y_wide:_(s32) = COPY $w1
%x:_(s1) = G_TRUNC %x_wide %x:_(s1) = G_TRUNC %x_wide
%y:_(s1) = G_TRUNC %y_wide %y:_(s1) = G_TRUNC %y_wide
%hand1:_(s64) = G_SEXT %x(s1) %hand1:_(s64) = G_SEXT %x(s1)
%hand2:_(s64) = G_SEXT %y(s1) %hand2:_(s64) = G_SEXT %y(s1)
%logic_op:_(s64) = G_OR %hand1, %hand2 %logic_op:_(s64) = G_OR %hand1, %hand2
$x0 = COPY %logic_op(s64) $x0 = COPY %logic_op(s64)
RET_ReallyLR implicit $x0 RET_ReallyLR implicit $x0
...
---
name: dont_combine_physreg
tracksRegLiveness: true
legalized: true
body: |
bb.0:
liveins: $w0, $w1
; Post-legalization, we must not crash here.
;
; CHECK-LABEL: name: dont_combine_physreg
%x:_(s32) = COPY $w0
%y:_(s32) = COPY $w1
%logic_op:_(s32) = G_OR %x, %y
$w0 = COPY %logic_op(s32)
RET_ReallyLR implicit $w0