Page MenuHomePhabricator

Refactor code for adding runtime checks in vectorizer.
AbandonedPublic

Authored by mzolotukhin on Apr 28 2015, 4:56 PM.

Details

Summary

Currently this code is very hard to manage:

  • variables defined in the beginning of the function might be used several screens later
  • Code for adding checks isn't uniform enough (some use Instruction::Create, others use IRBuilder)
  • Code for adding one check is tangled with code for adding next check, because we split basic block only at that time.

I tried to fix this issues, and hopefully that improves the code readability.

Diff Detail

Event Timeline

mzolotukhin retitled this revision from to Refactor code for adding runtime checks in vectorizer..
mzolotukhin updated this object.
mzolotukhin edited the test plan for this revision. (Show Details)
mzolotukhin added a subscriber: Unknown Object (MLST).

Any reason we can't use the IRBuilder instead of BranchInst::Create?

A side effect is that it would make it a little easier to make:

br i1 true, label %scalar.ph, label %overflow.checked

simply:

br label %scalar.ph

by specializing the builder's CreateCondBr to "Do the Right Thing (TM)" when given a constant condition.

Hi David,
No actual reason for not using builder for branches too, except that it wasn't used there before (this part was uniform in code for all checks, so I kept it as is).

  • Use IRBuilder instead of BranchInst::Create.
  • Fix a bug in code generation for StrideCheck.
hfinkel edited edge metadata.May 23 2015, 6:04 PM

I like the BypassBlock -> VectorPH rename (although I'd prefer it be named VectorPreHeader); can you please just commit that and then rebase the rest of the patch on that. It is a separable change.

Hi Hal,

The thing is that VectorPH already exists in the original code. Actually, the main points of this patch was to make this blocks structure more apparent - rather than having BypassBlock, LastBypassBlock, VectorPH and others, I'd prefer to have only VectorPH.

Another issue that I tried to solve here is the following. When we add a new runtime check in the original code, we

  1. Add instructions for new check to the basic block with previous check. E.g.:
std::tie(FirstCheckInst, MemRuntimeCheck) = Legal->getLAI()->addRuntimeCheck(LastBypassBlock->getTerminator());
  1. LastBypassBlock now contains instructions for both previous and new check. Split it:
BasicBlock *CheckBlock = LastBypassBlock->splitBasicBlock(FirstCheckInst, "vector.memcheck");
  1. Add a branch instruction based on the *previous* check:
Instruction *OldTerm = LastBypassBlock->getTerminator();
BranchInst::Create(MiddleBlock, CheckBlock, Cmp, OldTerm);
OldTerm->eraseFromParent();
  1. Save the *current* check - it will be used for creating branch when we create a *next* check.
Cmp = StrideCheck;

So, I tried to remove this dependency between different checks and make code responsible for generation a single check self-dependent and isolated.

PS: I'm fine with renaming VectorPH to VectorPreHeader, I'll do that along with rebasing on top of the trunk.

mzolotukhin edited edge metadata.
  • Rename VectorPH to VectorPreHeader.

Please commit the BypassBlock -> VectorPreHeader rename now, and then rebase this patch. That will make the remainder easier to review.

test/Transforms/LoopVectorize/induction.ll
116

You've removed the check for the icmp. Why?

Hi Hal,

I can't commit 's/BypassBlock/VectorPreHeader/' since the original code already has VectorPH, and having both VectorPH and VectorPreHeader simultaneously would be even more confusing. I'll try to remove as many other changes as possible though, and keep only the changes needed for this rename in a separate patch.

Thanks,
Michael

test/Transforms/LoopVectorize/induction.ll
116

I started using IRBuilder, and it folded this expression.

Hi Hal,

I split the patch to 4 parts, which hopefully would be easier to understand.

0001 - This is the main patch. Here we do most of the code restructuring (but I tried to keep other changes out of it).
0002 - Move some variable initializations closer to their (single) use. It saves some scrolling when reading the code.
0003 - Use IRBuilder for creating branches. Adjust the test case in which we started to fold a constant ‘icmp’.
0004 - Rename BypassBlock to VectorPreHeader. That’s the change you asked to commit, but it can’t be applied before 0001 patch.

Hi Hal,

I uploaded the first (biggest) part to phabricator in D10097 - hope this helps with review.
The other 3 parts should be trivial, so I didn't upload them.

Thanks,
Michael

  • Original Message -----

From: "Michael Zolotukhin" <mzolotukhin@apple.com>
To: mzolotukhin@apple.com, hfinkel@anl.gov
Cc: "david majnemer" <david.majnemer@gmail.com>, llvm-commits@cs.uiuc.edu
Sent: Wednesday, May 27, 2015 8:16:24 PM
Subject: Re: [PATCH] Refactor code for adding runtime checks in vectorizer.

Hi Hal,

I split the patch to 4 parts, which hopefully would be easier to
understand.

0001 - This is the main patch. Here we do most of the code

I'll review this one in its associated thread.

restructuring (but I tried to keep other changes out of it).
0002 - Move some variable initializations closer to their (single)
use. It saves some scrolling when reading the code.

LGTM.

0003 - Use IRBuilder for creating branches. Adjust the test case in
which we started to fold a constant ‘icmp’.

LGTM.

0004 - Rename BypassBlock to VectorPreHeader. That’s the change you
asked to commit, but it can’t be applied before 0001 patch.

LGTM.

-Hal

http://reviews.llvm.org/D9332

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/
mzolotukhin abandoned this revision.Jul 15 2015, 2:36 PM

Slightly changed variant of this refactoring has been committed (see D10097).