Page MenuHomePhabricator

[SLP] Factor out scalar type as a getter. NFC
Needs ReviewPublic

Authored by eopXD on Fri, May 27, 7:48 AM.

Diff Detail

Unit TestsFailed

TimeTest
80 msx64 debian > LLVM.Transforms/PhaseOrdering/X86::vector-reductions-logical.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -O2 -S < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Transforms/PhaseOrdering/X86/vector-reductions-logical.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Transforms/PhaseOrdering/X86/vector-reductions-logical.ll

Event Timeline

eopXD created this revision.Fri, May 27, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, May 27, 7:48 AM
eopXD requested review of this revision.Fri, May 27, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, May 27, 7:48 AM
RKSimon added inline comments.Fri, May 27, 9:42 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
221

(style)

if (auto*SI = dyn_cast<StoreInst>(V))
  return SI->getValueOperand()->getType();
if (auto *CI = dyn_cast<CmpInst>(V))
  return CI->getOperand(0)->getType();
if (auto *IE = dyn_cast<InsertElementInst>(V))
  return IE->getOperand(1)->getType();
return V->getType();
eopXD retitled this revision from [SLP][NFC] Factor out scalar type as a getter to [SLP] Factor out scalar type as a getter. NFC.Fri, May 27, 3:15 PM
eopXD updated this revision to Diff 432654.Fri, May 27, 3:18 PM

Update code

  • Address comment
eopXD marked an inline comment as done.Fri, May 27, 3:18 PM

Is this really a NFC? Could some of the cases that just call getType() now call one of the dyn_cast variants?

Anyway, no objections from me - @ABataev any comments?

eopXD added a comment.Mon, May 30, 1:59 PM

Thank you @RKSimon for raising this question.

Regarding the code that calls ->getType() and don't have dynamic casts...

  • Line 3494 can only be entered when instruction is a Instruction::Load.
  • Use of line 3614 can only be used when TE.Scalars consist of Load instructions.
  • Line 4909 is under case Instruction::Load
  • Line 11147 is under the reduction part of SLP, by the cases in RecurKind I think load / store / cmp instructions are not involved here.

With all this said, I am not sure if any existing code will cause this "none functional change intended" patch to fail.

ABataev added inline comments.Tue, May 31, 3:47 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7195

What if we gather cmps here?

10070

What if we vectorize list of cmps here?