This is an archive of the discontinued LLVM Phabricator instance.

Differential D92548

[CodeGen] Modify the refineIndexType(...)'s code to fix a bug in D90942.
ClosedPublic

Authored by yubing on Dec 3 2020, 12:18 AM.

Details

Reviewers
pengfei
kmclaughlin
craig.topper
LuoYuanke
Commits
rGeee30a6dceb6: [CodeGen] Modify the refineIndexType(...)'s code to fix a bug in D90942.
Summary

In previous code, when refineIndexType(...) is called and Index is undef, Index.getOperand(0) will raise a assertion fail.

Diff Detail

Event Timeline

yubing created this revision.Dec 3 2020, 12:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2020, 12:18 AM
Herald added subscribers: llvm-commits, ecnelises, pengfei and 2 others. · View Herald Transcript
yubing requested review of this revision.Dec 3 2020, 12:18 AM
yubing added reviewers: pengfei, kmclaughlin, craig.topper, LuoYuanke.Dec 3 2020, 12:20 AM
yubing updated this revision to Diff 309174.Dec 3 2020, 12:29 AM

Delete some useles code

sdesmalen added a subscriber: sdesmalen.Dec 3 2020, 1:13 AM

Thanks for finding and fixing this!

Can the test be simplified by using <4 x float> instead of <48 x float>?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9416

Probably better to explicitly check if Index is not a zero/sign extend, instead of checking whether it has any operands.

Harbormaster completed remote builds in B80921: Diff 309174.Dec 3 2020, 1:18 AM
Harbormaster completed remote builds in B80920: Diff 309172.Dec 3 2020, 1:50 AM
kmclaughlin mentioned this in D90942: [SVE][CodeGen] Improve codegen of scalable masked scatters.Dec 3 2020, 2:30 AM
yubing updated this revision to Diff 309246.Dec 3 2020, 6:52 AM

Modified some code according to comments

yubing added a comment.Dec 3 2020, 6:53 AM
In D92548#2430468, @sdesmalen wrote:

Thanks for finding and fixing this!

Can the test be simplified by using <4 x float> instead of <48 x float>?

Hi, thanks for comments. I can't change into <4 x float>.
I could change to <24 x float> because The smallest type to reproduce this bug is <24 x float>.

In the case of "<24 x float>", the scattered index is represented by a buildvector of<24 x i64> .
During LegalizeType procedure, The scattered index, aka, a buildvector of<24 x i64>will be widened into buildvector (<32 x i64>) whose last 8 elements are undef.
Then, <32 x i64> is split into two buildvectors(16 x i64) , one of which carries 8 undef.
Then, <16 x i64> is split into two buildvectors(8 x i64), one of which carries 8 undef, which means this buildvector will become a v8i64 undef(That's the masked.scatter's index which cause assertion fail).

Any thoughts?

yubing marked an inline comment as done.Dec 3 2020, 6:53 AM
pengfei added inline comments.Dec 3 2020, 7:01 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9423

How about just use Index = Index.getOperand(0)

Harbormaster completed remote builds in B80955: Diff 309246.Dec 3 2020, 7:30 AM
yubing updated this revision to Diff 309418.Dec 3 2020, 5:42 PM

Modified some code according Pengfei's comments

yubing marked an inline comment as done.Dec 3 2020, 5:43 PM
Harbormaster completed remote builds in B81046: Diff 309418.Dec 3 2020, 6:25 PM
yubing retitled this revision from [CodeGen] Check if Index's OperandNum == 0 when refineIndexType(...) is called This is a bugfix for D90942 to [CodeGen] Modify the refineIndexType(...)'s code to fix a bug in D90942..Dec 3 2020, 6:54 PM
yubing edited the summary of this revision. (Show Details)
yubing added a comment.Dec 3 2020, 9:57 PM

PING?

pengfei accepted this revision.Dec 3 2020, 10:22 PM

LGTM.

This revision is now accepted and ready to land.Dec 3 2020, 10:22 PM
Closed by commit rGeee30a6dceb6: [CodeGen] Modify the refineIndexType(...)'s code to fix a bug in D90942. (authored by yubing). · Explain WhyDec 6 2020, 4:50 PM
This revision was automatically updated to reflect the committed changes.
yubing added a commit: rGeee30a6dceb6: [CodeGen] Modify the refineIndexType(...)'s code to fix a bug in D90942..

Revision Contents

PathSize
llvm/
lib/
CodeGen/
SelectionDAG/
2 lines
test/
CodeGen/
X86/
49 lines

Diff 309172

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
//===- DAGCombiner.cpp - Implement a DAG node combiner --------------------===// //===- DAGCombiner.cpp - Implement a DAG node combiner --------------------===//
Lint: Lint
Inline Actions

clang-format not found in user's PATH; not linting file.

Lint: Lint: clang-format not found in user's PATH; not linting file.
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// //
▲ Show 20 Lines • Show All 9,399 Lines • ▼ Show 20 Linesbool refineUniformBase(SDValue &BasePtr, SDValue &Index, SelectionDAG &DAG) {
Index = Index.getOperand(1); Index = Index.getOperand(1);
return true; return true;
} }
// Fold sext/zext of index into index type. // Fold sext/zext of index into index type.
bool refineIndexType(MaskedScatterSDNode *MSC, SDValue &Index, bool Scaled, bool refineIndexType(MaskedScatterSDNode *MSC, SDValue &Index, bool Scaled,
SelectionDAG &DAG) { SelectionDAG &DAG) {
const TargetLowering &TLI = DAG.getTargetLoweringInfo(); const TargetLowering &TLI = DAG.getTargetLoweringInfo();
if(Index.getNumOperands() == 0)
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-  if(Index.getNumOperands() == 0)
+  if (Index.getNumOperands() == 0)
Lint: Pre-merge checks: clang-format: please reformat the code ``` - if(Index.getNumOperands() == 0) + if (Index.
sdesmalenUnsubmitted
Done
ReplyInline Actions

Probably better to explicitly check if Index is not a zero/sign extend, instead of checking whether it has any operands.

sdesmalen: Probably better to explicitly check if `Index` is not a zero/sign extend, instead of checking…
return false;
SDValue Op = Index.getOperand(0); SDValue Op = Index.getOperand(0);
if (Index.getOpcode() == ISD::ZERO_EXTEND) { if (Index.getOpcode() == ISD::ZERO_EXTEND) {
MSC->setIndexType(Scaled ? ISD::UNSIGNED_SCALED : ISD::UNSIGNED_UNSCALED); MSC->setIndexType(Scaled ? ISD::UNSIGNED_SCALED : ISD::UNSIGNED_UNSCALED);
if (TLI.shouldRemoveExtendFromGSIndex(Op.getValueType())) { if (TLI.shouldRemoveExtendFromGSIndex(Op.getValueType())) {
Index = Op; Index = Op;
pengfeiUnsubmitted
Done
ReplyInline Actions

How about just use Index = Index.getOperand(0)

pengfei: How about just use `Index = Index.getOperand(0)`
return true; return true;
} }
} }
if (Index.getOpcode() == ISD::SIGN_EXTEND) { if (Index.getOpcode() == ISD::SIGN_EXTEND) {
MSC->setIndexType(Scaled ? ISD::SIGNED_SCALED : ISD::SIGNED_UNSCALED); MSC->setIndexType(Scaled ? ISD::SIGNED_SCALED : ISD::SIGNED_UNSCALED);
if (TLI.shouldRemoveExtendFromGSIndex(Op.getValueType())) { if (TLI.shouldRemoveExtendFromGSIndex(Op.getValueType())) {
Index = Op; Index = Op;
▲ Show 20 Lines • Show All 13,192 Lines • Show Last 20 Lines

llvm/test/CodeGen/X86/combine-undef-index-mscatter.ll

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; REQUIRES: intel_feature_isa_fp16
; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=avx512f | FileCheck %s
define void @main(<48 x float*> %elemBasePtr.)
; CHECK-LABEL: main:
; CHECK: # %bb.0: # %entry
; CHECK-NEXT: vmovq %rcx, %xmm0
; CHECK-NEXT: vmovq %rdx, %xmm1
; CHECK-NEXT: vpunpcklqdq {{.*#+}} xmm0 = xmm1[0],xmm0[0]
; CHECK-NEXT: vmovq %rsi, %xmm1
; CHECK-NEXT: vmovq %rdi, %xmm2
; CHECK-NEXT: vpunpcklqdq {{.*#+}} xmm1 = xmm2[0],xmm1[0]
; CHECK-NEXT: vinserti128 $1, %xmm0, %ymm1, %ymm0
; CHECK-NEXT: vmovq %r9, %xmm1
; CHECK-NEXT: vmovq %r8, %xmm2
; CHECK-NEXT: vpunpcklqdq {{.*#+}} xmm1 = xmm2[0],xmm1[0]
; CHECK-NEXT: vinserti128 $1, {{[0-9]+}}(%rsp), %ymm1, %ymm1
; CHECK-NEXT: vinserti64x4 $1, %ymm1, %zmm0, %zmm0
; CHECK-NEXT: vmovups {{[0-9]+}}(%rsp), %zmm1
; CHECK-NEXT: vmovups {{[0-9]+}}(%rsp), %zmm2
; CHECK-NEXT: vmovups {{[0-9]+}}(%rsp), %zmm3
; CHECK-NEXT: vmovups {{[0-9]+}}(%rsp), %zmm4
; CHECK-NEXT: vmovups {{[0-9]+}}(%rsp), %zmm5
; CHECK-NEXT: kxnorw %k0, %k0, %k1
; CHECK-NEXT: vmovaps {{.*#+}} ymm6 = [8.33005607E-1,8.435871E-1,1.69435993E-1,8.33005607E-1,8.435871E-1,1.69435993E-1,8.33005607E-1,8.435871E-1]
; CHECK-NEXT: kxnorw %k0, %k0, %k2
; CHECK-NEXT: vscatterqps %ymm6, (,%zmm0) {%k2}
; CHECK-NEXT: vmovaps {{.*#+}} ymm0 = [1.69435993E-1,8.33005607E-1,8.435871E-1,1.69435993E-1,8.33005607E-1,8.435871E-1,1.69435993E-1,8.33005607E-1]
; CHECK-NEXT: kxnorw %k0, %k0, %k2
; CHECK-NEXT: vscatterqps %ymm0, (,%zmm5) {%k2}
; CHECK-NEXT: vmovaps {{.*#+}} ymm5 = [8.435871E-1,1.69435993E-1,8.33005607E-1,8.435871E-1,1.69435993E-1,8.33005607E-1,8.435871E-1,1.69435993E-1]
; CHECK-NEXT: kxnorw %k0, %k0, %k2
; CHECK-NEXT: vscatterqps %ymm5, (,%zmm4) {%k2}
; CHECK-NEXT: kxnorw %k0, %k0, %k2
; CHECK-NEXT: vscatterqps %ymm6, (,%zmm3) {%k2}
; CHECK-NEXT: kxnorw %k0, %k0, %k2
; CHECK-NEXT: vscatterqps %ymm0, (,%zmm2) {%k2}
; CHECK-NEXT: vscatterqps %ymm5, (,%zmm1) {%k1}
; CHECK-NEXT: vzeroupper
; CHECK-NEXT: retq
{
entry:
call void @llvm.masked.scatter.v48f32.v48p0f32(<48 x float> <float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000, float 0x3FEAA7FB60000000, float 0x3FEAFEAA60000000, float 0x3FC5B01420000000>, <48 x float*> %elemBasePtr., i32 4, <48 x i1> <i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true>)
ret void
}
; Function Attrs: nofree nosync nounwind willreturn writeonly
declare void @llvm.masked.scatter.v48f32.v48p0f32(<48 x float>, <48 x float*>, i32 immarg, <48 x i1>)