[PATCH] D32338: InstructionSimplify: Canonicalize shuffle operands. NFC-ish.

[Zvi Rackover via Phabricator via llvm-commits]

zvi added inline comments.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:4140-4142
-  if (isa<UndefValue>(Mask))
-    return UndefValue::get(RetTy);
-
----------------
spatel wrote:
> I got confused by the order of the patches/dependencies. This patch is proposing to remove this check that would be added by D32293. That's because the code in this patch will tell us that MaskSelects0 && MaskSelects1 are false, and that will then fall into ConstantFoldShuffleVectorInstruction and get folded?
> 
> I would've just left this check here as an early exit for the easy case.
> 
> FWIW, this may just be a case of too many cooks in the kitchen. :)
> I think the end result of these 2 patches covers everything that we want in the current set of regression tests.
> 
> 
Sorry about the excessive patches, will try to structure them better next time :)

The answer to your question is yes.

I thought it would be best to remove this early exit from fear of premature optimization as i don't have any data on how often this pattern occurs in real-life workloads (though from looking at the lit tests it would appear to happen quite often - but these are engineered corner cases).
I have no problem with leaving this early-exit in place..


Repository:
  rL LLVM

https://reviews.llvm.org/D32338