llvm dev - Jan 2017 - DAGLegalizer Issue

Hello,

As I try to compile some generated llvm IR, I have an issue with the
shuffle instruction on FloatingPoint reaching an assert in the DAGLegalizer.
I try to produce a patch to fix it and I wonder what is the best way to
handle it.

Now the problem:
There is a shuffle transformation like:
; fold: (shuffle (bitcast (BINOP A, B)), Undef, <Mask>)
->                  
;       (shuffle (BINOP (bitcast A), (bitcast B)), Undef, <Mask>)

For X86 with BINOP in (FADD, FMUL, FSUB, ADD, MUL, SUB).
It seems OK and I don't want to change that but I have a case where
bitcast is done from float to int. In this case, we can't perform this
transformation
as FADD can't be apply on integers.

I could have fix it here and say : FADD, FMUL, FSUB can be switch with
float bitcast only and ADD/MUL/SUB with intergers bitcast  only. But few
lines above there is an if with:
TLI.isOperationLegal(Opcode, VT).
It means that Opcode == FADD and VT type == Integers return True (so is
Legal) and it doesn't looks correct to me.
So I wanted to registers op/type couple as illegal with something like

for (MVT VT : MVT::integer_valuetypes()) {
   setOperationAction(ISD::FADD, VT , Custom);
   setOperationAction(ISD::FSUB, VT , Custom);
   setOperationAction(ISD::FMUL, VT , Custom);
}

But "Custom" doesn't really means illegal so what should I use? I
can
keep custom and fill the lowerOperation switch but as it is a corner
case where we perform transformation after legalizing the DAG, I am not
sure it is the good solution.
As I am certainly missing something, I rely on you to give me more
informations on the lowering process.

Thanks,
Pierrick

On 1/17/2017 5:04 AM, pbrunet via llvm-dev wrote:
> Hello,
>
> As I try to compile some generated llvm IR, I have an issue with the
> shuffle instruction on FloatingPoint reaching an assert in the
DAGLegalizer.
> I try to produce a patch to fix it and I wonder what is the best way to
> handle it.
>
> Now the problem:
> There is a shuffle transformation like:
> ; fold: (shuffle (bitcast (BINOP A, B)), Undef, <Mask>)
> ->
> ;       (shuffle (BINOP (bitcast A), (bitcast B)), Undef, <Mask>)
>
> For X86 with BINOP in (FADD, FMUL, FSUB, ADD, MUL, SUB).
This is combineShuffle in X86ISelLowering.cpp?
> It seems OK and I don't want to change that but I have a case where
> bitcast is done from float to int. In this case, we can't perform this
> transformation
> as FADD can't be apply on integers.
Actually, you can still perform the transformation; for ADD, you just 
need to compute an equivalent integer type to use for the binop, then 
insert an extra bitcast afterwards.

On a side-note, I'm pretty sure the transform isn't legal for 
floating-point vectors; you can get away with extending an integer by 
putting garbage into the high bits, but you can't extend a 
floating-point value that way
> I could have fix it here and say : FADD, FMUL, FSUB can be switch with
> float bitcast only and ADD/MUL/SUB with intergers bitcast  only. But few
> lines above there is an if with:
> TLI.isOperationLegal(Opcode, VT).
> It means that Opcode == FADD and VT type == Integers return True (so is
> Legal) and it doesn't looks correct to me.
This is arguably wrong, but it doesn't matter because we shouldn't be 
trying to FADD integers anyway.

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project