[PATCH] D77136: [X86] Do not assume types are legal in getFauxShuffleMask
Bjorn Pettersson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 31 06:37:11 PDT 2020
bjope marked 2 inline comments as done.
bjope added inline comments.
================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:7528
+ return false;
MVT SrcVT = Src.getSimpleValueType();
unsigned NumSrcElts = SrcVT.getVectorNumElements();
----------------
RKSimon wrote:
> Nice catch! We actually need to be more thorough, matching what we do for the extensions (further down the function):
> ```
> SDValue Src = N.getOperand(0);
> EVT SrcVT = Src.getValueType();
>
> // Truncated source must be a simple vector.
> if (!SrcVT.isSimple() || (SrcVT.getSizeInBits() % 128) != 0 ||
> (SrcVT.getScalarSizeInBits() % 8) != 0)
> return false;
> ```
Ok, I'll copy that.
I was wondering if perhaps this whole thing should be avoided until we have legalized types? But maybe we want to do these combines earlier than that for some reason. But then I guess we still need the tests on bit sizes, so it wouldn't be enough.
================
Comment at: llvm/test/CodeGen/X86/shuffle-combine-crash-3.ll:60
+ %9 = and i1 %8, %7
+ br i1 %9, label %for.cond.9, label %if.then.i
+
----------------
RKSimon wrote:
> This can probably be reduced/cleaned up more?
I did some attempts to reduce it (for example removing the first insertelement/shufflevector instructions using input arguments, but then I got a lot more asm instructions in the output).
But given the feedback that the code change is reasonable, then I feel that I can invest some more time on reducing the test case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77136/new/
https://reviews.llvm.org/D77136
More information about the llvm-commits
mailing list