This is an archive of the discontinued LLVM Phabricator instance.

Differential D87423

[GVN] Account for masked loads/stores depending on load/store instructions
ClosedPublic

Authored by kparzysz on Sep 9 2020, 4:16 PM.

Details

Reviewers
fhahn
asbirlea
Commits
rG8a08740db6e1: [GVN] Account for masked loads/stores depending on load/store instructions
Summary

This is a case where an intrinsic depends on a non-call instruction.

This fixes the testcase from https://reviews.llvm.org/D87061.

Diff Detail

Event Timeline

kparzysz created this revision.Sep 9 2020, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 4:16 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
kparzysz requested review of this revision.Sep 9 2020, 4:16 PM
kparzysz mentioned this in D87061: Handle masked loads and stores in MemoryLocation/Dependence.
Harbormaster completed remote builds in B71158: Diff 290842.Sep 9 2020, 4:47 PM
fhahn accepted this revision.Sep 10 2020, 6:16 AM

LGTM thanks. Please run clang-format-diff before committing.

llvm/lib/Transforms/Scalar/GVN.cpp
417

nit: I think it is more common in this file to use !local_cdep instead of local_cdefp == nullptr.

llvm/test/Transforms/GVN/masked-load-store-vn-crash.ll
5

nit: I think the dso_local fastcc unnamed_addr #0 can eb removed.

This revision is now accepted and ready to land.Sep 10 2020, 6:16 AM
This revision was landed with ongoing or failed builds.Sep 10 2020, 8:58 AM
Closed by commit rG8a08740db6e1: [GVN] Account for masked loads/stores depending on load/store instructions (authored by kparzysz). · Explain Why
This revision was automatically updated to reflect the committed changes.
kparzysz added a commit: rG8a08740db6e1: [GVN] Account for masked loads/stores depending on load/store instructions.

Revision Contents

PathSize
llvm/
lib/
Transforms/
Scalar/
7 lines
test/
Transforms/
GVN/
20 lines

Diff 290842

llvm/lib/Transforms/Scalar/GVN.cpp

Show First 20 Lines • Show All 404 Lines • ▼ Show 20 Lines if (AA->doesNotAccessMemory(C)) {
MemDepResult local_dep = MD->getDependency(C); MemDepResult local_dep = MD->getDependency(C);
if (!local_dep.isDef() && !local_dep.isNonLocal()) { if (!local_dep.isDef() && !local_dep.isNonLocal()) {
valueNumbering[C] = nextValueNumber; valueNumbering[C] = nextValueNumber;
return nextValueNumber++; return nextValueNumber++;
} }
if (local_dep.isDef()) { if (local_dep.isDef()) {
CallInst* local_cdep = cast<CallInst>(local_dep.getInst()); // For masked load/store intrinsics, the local_dep may actully be
// a normal load or store instruction.
CallInst* local_cdep = dyn_cast<CallInst>(local_dep.getInst());
Lint: Pre-merge checks
Inline Actions

clang-tidy: warning: invalid case style for variable 'local_cdep' [readability-identifier-naming]
not useful
clang-format: please reformat the code

-      CallInst* local_cdep = dyn_cast<CallInst>(local_dep.getInst());
+      CallInst *local_cdep = dyn_cast<CallInst>(local_dep.getInst());
Lint: Pre-merge checks: clang-tidy: warning: invalid case style for variable 'local_cdep' [readability-identifier…
if (local_cdep->getNumArgOperands() != C->getNumArgOperands()) { if (local_cdep == nullptr ||
fhahnUnsubmitted
Not Done
ReplyInline Actions

nit: I think it is more common in this file to use !local_cdep instead of local_cdefp == nullptr.

fhahn: nit: I think it is more common in this file to use `!local_cdep` instead of `local_cdefp ==…
local_cdep->getNumArgOperands() != C->getNumArgOperands()) {
valueNumbering[C] = nextValueNumber; valueNumbering[C] = nextValueNumber;
return nextValueNumber++; return nextValueNumber++;
} }
for (unsigned i = 0, e = C->getNumArgOperands(); i < e; ++i) { for (unsigned i = 0, e = C->getNumArgOperands(); i < e; ++i) {
uint32_t c_vn = lookupOrAdd(C->getArgOperand(i)); uint32_t c_vn = lookupOrAdd(C->getArgOperand(i));
uint32_t cd_vn = lookupOrAdd(local_cdep->getArgOperand(i)); uint32_t cd_vn = lookupOrAdd(local_cdep->getArgOperand(i));
if (c_vn != cd_vn) { if (c_vn != cd_vn) {
▲ Show 20 Lines • Show All 2,457 Lines • Show Last 20 Lines

llvm/test/Transforms/GVN/masked-load-store-vn-crash.ll

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -gvn -S < %s | FileCheck %s
@file_mask = external global [8 x i64], align 32
define dso_local fastcc void @test() unnamed_addr #0 {
fhahnUnsubmitted
Not Done
ReplyInline Actions

nit: I think the dso_local fastcc unnamed_addr #0 can eb removed.

fhahn: nit: I think the `dso_local fastcc unnamed_addr #0` can eb removed.
; CHECK-LABEL: @test(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[WIDE_MASKED_LOAD_1_I:%.*]] = tail call <4 x i64> @llvm.masked.load.v4i64.p0v4i64(<4 x i64>* nonnull bitcast (i64* getelementptr inbounds ([8 x i64], [8 x i64]* @file_mask, i64 0, i64 7) to <4 x i64>*), i32 8, <4 x i1> <i1 true, i1 false, i1 false, i1 false>, <4 x i64> undef)
; CHECK-NEXT: unreachable
;
entry:
%wide.masked.load.1.i = tail call <4 x i64> @llvm.masked.load.v4i64.p0v4i64(<4 x i64>* nonnull bitcast (i64* getelementptr inbounds ([8 x i64], [8 x i64]* @file_mask, i64 0, i64 7) to <4 x i64>*), i32 8, <4 x i1> <i1 true, i1 false, i1 false, i1 false>, <4 x i64> undef) #2
%.pre392.i = load i64, i64* getelementptr inbounds ([8 x i64], [8 x i64]* @file_mask, i64 0, i64 7), align 8
%or156.4.i = or i64 %.pre392.i, undef
%wide.masked.load614.1.i = tail call <4 x i64> @llvm.masked.load.v4i64.p0v4i64(<4 x i64>* nonnull bitcast (i64* getelementptr inbounds ([8 x i64], [8 x i64]* @file_mask, i64 0, i64 7) to <4 x i64>*), i32 8, <4 x i1> <i1 true, i1 false, i1 false, i1 false>, <4 x i64> undef) #2
unreachable
}
; Function Attrs: argmemonly nounwind readonly willreturn
declare <4 x i64> @llvm.masked.load.v4i64.p0v4i64(<4 x i64>*, i32 immarg, <4 x i1>, <4 x i64>)