[PATCH] D16137: AVX512: VMOVDQU8/16/32/64 (load) intrinsic implementation.

[Demikhovsky, Elena via llvm-commits]

-  Elena


-----Original Message-----
From: Mitch Bodart [mailto:mitch.l.bodart at intel.com]
Sent: Tuesday, January 19, 2016 19:04
To: Breger, Igor <igor.breger at intel.com>; Demikhovsky, Elena <elena.demikhovsky at intel.com>; Badouh, Asaf <asaf.badouh at intel.com>; Kreitzer, David L <david.l.kreitzer at intel.com>; Bodart, Mitch L <mitch.l.bodart at intel.com>
Cc: llvm-commits at lists.llvm.org
Subject: Re: [PATCH] D16137: AVX512: VMOVDQU8/16/32/64 (load) intrinsic implementation.

mbodart added a comment.

Hi Asaf,

Thanks for the answers.

Note that SelectionDAGLegalize::LegalizeOp, in the case of custom lowering, already handles the case of multiple results, albeit with this FIXME comment:

  // FIXME: The handling for custom lowering with multiple results is
  // a complete mess.

So it is not clear to me why the new masked load intrinsics require additional changes, especially since we have existing masked load intrinsics with a chain result.
I'm guessing it's because legalization of the I64 mask operand on IA32 does not go through this SelectionDAGLegalize::LegalizeOp path.  Is that correct?
[Demikhovsky, Elena] Yes, right. The existing masked_load just does not have i64 argument that requires legalization.

Assuming we do need an X86 override of LowerOperationWrapper, then my next thought would be to try and fix the FILD issue in X86TargetLowering::FP_TO_INTHelper.  But I don't know how to express there, in Selection Dag representation, that only one result is needed.
If you know how to do that, then that would be a preferable solution.
[Demikhovsky, Elena] Igor tried. The created node (load or memory intrinsic) has 2 results – value and chain. But this is Ok. Look at ReplaceAllUsesWith.
This function is used on all stages of DAG modification. If “From” has 1 values and “To” has 2 values then the second value of “To” is dropped.
void SelectionDAG::ReplaceAllUsesWith(SDNode *From, const SDValue *To) {
  if (From->getNumValues() == 1)  // Handle the simple case efficiently.
    return ReplaceAllUsesWith(SDValue(From, 0), To[0]);


If we really need LowerOperationWrapper to discard the FILD's Chain operand, then I think we need to make it clear in that routine that it should only be happening for the FILD case, as in general it is not safe to simply drop a result.  So a source comment there describing the situation is needed.  And when we do need to drop a result, I think there should be an assertion checking exactly for the FILD case.  That is, we should only ever drop one result, and it should be a Chain, and if reasonable, we should check that the retained result is an FILD.  This will make it clear to developers why this code is needed, and thus make it easier to modify in the future.
[Demikhovsky, Elena] There is a special interface that allows to “drop” Res1. It goes to  ReplaceAllUsesWith() I pointed above.
SDValue CombineTo(SDNode *N, SDValue Res0, SDValue Res1,
                      bool AddTo = true) {
      SDValue To[] = { Res0, Res1 };
      return CombineTo(N, To, 2, AddTo);
    }

Thanks for updating the test functions!
One other thing I noticed about the test files is that most of them do not test for a 32-bit target.
That doesn't need to be added for this change set.  But as I64 masking has interesting behavior on IA32, it would be good to beef up testing there.

regards,

- mitch


Repository:
  rL LLVM

http://reviews.llvm.org/D16137




---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160119/fad8bdf4/attachment.html>