[PATCH] D101465: [RISCV] Lower splats of non-constant i1s as SETCCs

Mon May 3 01:42:10 PDT 2021

frasercrmck added inline comments.

================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1365
+      MVT InterVT = VT.changeVectorElementType(MVT::i8);
+      Splat = DAG.getSplatBuildVector(InterVT, DL, Splat);
+      SDValue Zero = DAG.getConstant(0, DL, InterVT);
----------------
craig.topper wrote:
> frasercrmck wrote:
> > craig.topper wrote:
> > > frasercrmck wrote:
> > > > craig.topper wrote:
> > > > > Don't we need to mask out all but bit 0 of Splat? I don't think those bits are defined.
> > > > Yeah I think you're right. I was going by the "zero or one" boolean contents contract which I thought covered us when the i1s were widened to XLEN but, thinking about it, that probably doesn't cover the operands of `BUILD_VECTOR` or `SPLAT_VECTOR`. Right?
> > > It applies to the output of SETCC or the expectations of SELECT. But the i1 could have been produced by a TRUNCATE for example in which case it wouldn't have to respect boolean contents. 
> > > 
> > > DAG combine should be able to delete the AND if the input came from SETCC, though X86 detects that case directly in lowering. See LowerBUILD_VECTORvXi1 right around the FIXME comment in that function.
> > Yeah, makes sense. I was hoping to hear something like that. I added a couple of tests such as `i1 zeroext` and where the `i1` comes from an `icmp` and I verified that those are correctly optimized. You should be able to see those further down somewhere.
> Your last comment make it sound like you uploaded another patch, but I only see one patch in the history.
Oh dear yeah I forgot to upload the latest diff. Thanks!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101465/new/

https://reviews.llvm.org/D101465