[PATCH 1/1] R600: Use TargetConstant in LowerConstantInitializer

[Matt Arsenault]

On May 19, 2014, at 9:38 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:

> On Sun, 2014-05-18 at 17:09 -0700, Matt Arsenault wrote:
>> On May 18, 2014, at 12:58 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
>> 
>>> We can only produce legal types at this stage.
>>> TargetConstant is an exception
>>> 
>>> Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu>
>>> ---
>>> 
>>> This fixes assertion failure (LegalizeDAG.cpp:1171) in one GEGL op.
>>> The alternative would be to use Constant of
>>> getSmallestLegalIntType(getPrimitiveSizeInBits) type.
>>> 
>>> lib/Target/R600/AMDGPUISelLowering.cpp | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/lib/Target/R600/AMDGPUISelLowering.cpp b/lib/Target/R600/AMDGPUISelLowering.cpp
>>> index 04924cf..d4c3191 100644
>>> --- a/lib/Target/R600/AMDGPUISelLowering.cpp
>>> +++ b/lib/Target/R600/AMDGPUISelLowering.cpp
>>> @@ -524,7 +524,7 @@ SDValue AMDGPUTargetLowering::LowerConstantInitializer(const Constant* Init,
>>>  if (const ConstantInt *CI = dyn_cast<ConstantInt>(Init)) {
>>>    EVT VT = EVT::getEVT(CI->getType());
>>>    PointerType *PtrTy = PointerType::get(CI->getType(), 0);
>>> -    return DAG.getStore(Chain, DL,  DAG.getConstant(*CI, VT), InitPtr,
>>> +    return DAG.getStore(Chain, DL,  DAG.getTargetConstant(*CI, VT), InitPtr,
>>>                 MachinePointerInfo(UndefValue::get(PtrTy)), false, false,
>>>                 TD->getPrefTypeAlignment(CI->getType()));
>>>  }
>>> @@ -532,7 +532,7 @@ SDValue AMDGPUTargetLowering::LowerConstantInitializer(const Constant* Init,
>>>  if (const ConstantFP *CFP = dyn_cast<ConstantFP>(Init)) {
>>>    EVT VT = EVT::getEVT(CFP->getType());
>>>    PointerType *PtrTy = PointerType::get(CFP->getType(), 0);
>>> -    return DAG.getStore(Chain, DL, DAG.getConstantFP(*CFP, VT), InitPtr,
>>> +    return DAG.getStore(Chain, DL, DAG.getTargetConstantFP(*CFP, VT), InitPtr,
>>>                 MachinePointerInfo(UndefValue::get(PtrTy)), false, false,
>>>                 TD->getPrefTypeAlignment(CFP->getType()));
>>>  }
>>> -- 
>>> 1.9.0
>> 
>> 
>> Needs a test
> 
> Good point. I have attached v2. However, it fails in the i32 case. For
> some reason i32 TargetConstant operand gets turned into Immediate and
> expandPostRAPseudo dies.
> I'm not sure where the bug is now. It's the conversion intentional
> (float, i8, i16 don't do that), or should the conversion be prevented?
> 
> I have also attached a small workaround that fixes the assertion
> failure, but it looks rather ugly.

So I think this is sort of correct, but I don’t know why this function is doing what it’s doing. I’m pretty sure you don’t want to use TargetConstant to fix this. It’s copying the constant initializer onto the “stack”, but expanding it into a legal type so the private memory copy doesn’t have the exact same layout as in the constant. I don’t know if that’s OK or not since I don’t know why this is copying it.

Besides that, I think it would be better to get the type to use with TLI.getRegisterType() rather than using getSmallestLegalIntType. You should also keep the variable as MVT since you’re trying to only use legal types here.

-Matt