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

[Mitch Bodart via llvm-commits]

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?

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.

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.

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