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

[Elena Demikhovsky via llvm-commits]

delena added a comment.

In http://reviews.llvm.org/D16137#330156, @mbodart wrote:

> 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?


The existing mask load intrinsics does not have i64 operand. (You mean @llvm.masked.load(), right).
They go to promote/widening.

> 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.


This workaround is not necessary. There are many places in the code, where we convert a one-result-node to two-result-node. 
It happens every time, when we use memory for node transformation. In all these cases we drop the second result. We even don't check that the second result is a chain. See ReplaceAllUsesWith() code:

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.

> 

> 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