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

Fraser Cormack via Phabricator via llvm-commits llvm-commits at lists.llvm.org
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



More information about the llvm-commits mailing list