[PATCH] D83654: [PowerPC] Support constrained vector fp/int conversion

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8638
 
   return DAG.getNode(Opc, dl, Op.getValueType(), Extend);
 }
----------------
uweigand wrote:
> qiucf wrote:
> > uweigand wrote:
> > > It would seem that this doesn't create a correct node when called with a STRICT version of Opc -- for one, in this case it will need to handle chains correctly (in and out).
> > > 
> > > It is strange that this was not detected by any tests -- is the coverage good enough?
> > > 
> > > The rest of the algorithm seems OK for the strict case, since it only introduces integer operations.
> > Yes, it was missed here. Thanks for comments!
> > 
> > This method is for custom lowering `v4i8/v4i16`. They are not set to `custom` in this patch so such cases will be expanded automatically. After setting them, it hits an assert since PPC doesn't override `TargetLowering::LowerOperationWrapper`. This can be done in later patches.
> > 
> > ```
> > if (SDValue Res = LowerOperation(SDValue(N, 0), DAG))
> >   Results.push_back(Res);
> > ```
> > 
> > BTW, the method looks strange to me, maybe we can fill rest of operands in base class implementation.
> Well, if the changes to this routine are never exercised without some additional changes, maybe they should be removed from this patch and added to the patch that'll introduce the first use?
> 
> On the other hand, I see above that at least for v4i1 the method **is** set to Custom, so is this routine used after all?  (Not sure when v4i1 is actually used here, a bool->float conversion looks rare ...   Are there test cases for v4i1?)
Oh, `v4i1` would be promoted to `v4i32`. So setting it to custom (both strict and non-strict) is actually meaningless.. I'll add the handling for `v4i16` and others along with tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83654