[PATCH] D32338: InstructionSimplify: Canonicalize shuffle operands. NFC-ish.
Zvi Rackover via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 21 09:26:32 PDT 2017
zvi added inline comments.
================
Comment at: lib/Analysis/InstructionSimplify.cpp:4148
for (unsigned i = 0; i != MaskNumElts; ++i) {
int Idx = ShuffleVectorInst::getMaskValue(Mask, i);
if (Idx == -1)
----------------
RKSimon wrote:
> You are calling ShuffleVectorInst::getMaskValue multiple times per element - better to call ShuffleVectorInst::getShuffleMask instead?
Right, this is the case even without this patch (see lines 4158 and 4188). Will add a parent patch to address that.
================
Comment at: lib/Analysis/InstructionSimplify.cpp:4170
+ // Canonicalization: if only one input vector is contant, it shall be the
+ // second one.
+ if (Op0Const && !Op1Const) {
----------------
RKSimon wrote:
> Add tests for this?
The tests are already there and this is supposed to be a NFC patch.
This canonoicalization allowed the removal of lines 4169-4171 which are a duplication of lines 4166-4168 (or lines 4188-4190 after the patch is applied).
One test-case that should give coverage is the function 'splat_operand3' in test/Transforms/InstSimplify/shufflevector.ll
================
Comment at: lib/Analysis/InstructionSimplify.cpp:4197
+ return nullptr;
// Check if every element of this shuffle can be mapped back to the
----------------
RKSimon wrote:
> Add an assert here to check that NewIndices has MaskNumElts elements
Thanks for pointing this out. I didn't really intend for the newly created mask to be illegal in the case it contains an undef. Will fix this.
Repository:
rL LLVM
https://reviews.llvm.org/D32338
More information about the llvm-commits
mailing list