This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] dropRedundantMaskingOfLeftShiftInput(): propagate undef shift amounts
ClosedPublic

Authored by lebedev.ri on Sep 30 2019, 12:35 PM.

Details

Summary

When we do ConstantExpr::getZExt(), that "extends" undef to 0,
which means that for patterns a/b we'd assume that we must not produce
any bits for that channel, while in reality we simply didn't care
about that channel - i.e. we don't need to mask it.

Diff Detail

Event Timeline

lebedev.ri created this revision.Sep 30 2019, 12:35 PM
lebedev.ri marked an inline comment as done.Sep 30 2019, 12:36 PM
lebedev.ri added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
120

I'm open to suggestions about this function and it's name.

I'd be okay with either spliting this up further if wanted, or moving this particular patch
into post-commit review mode; the only worrying thing to me here is the sanitizeUndefsTo() itself.

spatel added a comment.Oct 4 2019, 8:52 AM

I'd be okay with either spliting this up further if wanted, or moving this particular patch
into post-commit review mode; the only worrying thing to me here is the sanitizeUndefsTo() itself.

I'm a patch minimalist, so if we can split this up, let's do it.
We should not use "sanitize" in the function name because that term already has a special meaning in the clang/LLVM world.

I'd be okay with either spliting this up further if wanted, or moving this particular patch
into post-commit review mode; the only worrying thing to me here is the sanitizeUndefsTo() itself.

I'm a patch minimalist, so if we can split this up, let's do it.

Cool, will post in a sec..

We should not use "sanitize" in the function name because that term already has a special meaning in the clang/LLVM world.

Any suggestions? replaceUndefsWith?

lebedev.ri updated this revision to Diff 223232.Oct 4 2019, 9:11 AM
lebedev.ri retitled this revision from [InstCombine] dropRedundantMaskingOfLeftShiftInput(): change how we deal with mask to [InstCombine] dropRedundantMaskingOfLeftShiftInput(): propagate undef shift amounts.
lebedev.ri edited the summary of this revision. (Show Details)

Splitting into two patches.

spatel added inline comments.Oct 4 2019, 12:55 PM
llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
121–130

I suppose it's a matter of C++ familiarity, but this logic is not obvious to me.

I think it's easier to read as something like:

unsigned NumElts = CV->getType()->getVectorNumElements();
for (unsigned i = 0; i != NumElts; ++i) {
  Constant *EltC= CV->getOperand(i);
  NewOps[i] = match(EltC, m_Undef()) ? Replacement : EltC;
}
199–201

Probably worth explaining this more directly, and what if we make it consistent by replacing with -1?

// An extend of an undef value becomes zero because the 
// high bits are never completely unknown. Replace the
// the shift amount with -1 to ensure that the value remains
// undef when creating the subsequent shift op.
spatel added inline comments.Oct 4 2019, 12:59 PM
llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
120

Is it possible to get here with a scalar undef? If so, this doesn't replace it. If not, do we want to assert that we have a vector constant?

lebedev.ri updated this revision to Diff 223329.Oct 4 2019, 3:44 PM
lebedev.ri marked 5 inline comments as done.

Addressed review notes.

llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
120

Given that we get the constant via running a custom instsimplify
i wouldn't fully exclude the situation that we'd end with scalar undef here.
It's not *really* a correctness problem.
Let's not assert but also replace.

199–201

Good comments are always good :)
Not with -1 though.

lebedev.ri updated this revision to Diff 223331.Oct 4 2019, 4:00 PM

Upload correct patch this time.

spatel added inline comments.Oct 5 2019, 11:03 AM
llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
199–201

Why is using different variants based on bidwidth better? Or does -1 not work as a poisonous shift amount?

lebedev.ri added inline comments.Oct 5 2019, 11:11 AM
llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
199–201

In this branch - yes, i think -1 should work.
But in other branch we will negate that shift amount and add innermost shift bitwidth to it.
What i'm trying to say is, it is not so much about that the replacement shift amount *here*
will still produce undef, but it must do so when we actually perform the shift.
It seemed consistent to me to end up with same shift-by-bitwidth in both branches.

Let me know if that explanation does not make any sense.

spatel accepted this revision.Oct 6 2019, 6:38 AM

LGTM

llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
199–201

Ok, I see what you mean. It seems like we're missing some analysis that would handle this sort of thing for us cleanly, but I don't see where/how to implement it, so I won't hold this up.

This revision is now accepted and ready to land.Oct 6 2019, 6:38 AM
lebedev.ri marked 4 inline comments as done.Oct 6 2019, 7:20 AM

LGTM

Thank you for the review!

There's second half of the patch in D68470.
I did't post the actual patch adding trunc support yet, tests needed...

llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
199–201

Yeah..

This revision was automatically updated to reflect the committed changes.