Page MenuHomePhabricator

[RISCV] Refactory the existing CC_RISCV32 function to conform to the CCAssignFn type
Needs ReviewPublic

Authored by xiangzhai on Jan 3 2018, 5:03 AM.

Details

Reviewers
asb
Summary

Hi LLVM developers,

As Alex suggested, I refactory the existing CC_RISCV32 function to conform to the CCAssignFn type for D41653 Pass a real CCAssignFn to the ValueHandler, please review my patch, thanks a lot!

Regards,
Leslie Zhai

Diff Detail

Repository
rL LLVM

Event Timeline

xiangzhai created this revision.Jan 3 2018, 5:03 AM
asb added a comment.Jan 3 2018, 7:21 AM

Thanks for the patch Leslie. My main concern with this approach is it will fail to work when VarArg support is added (D40805). Conforming to the vararg ABI requires 1) knowing which arguments are fixed vs non-fixed (I currently just have an extra parameter for this in CC_RISCV, Mips handles this with MipsCCState), 2) knowing the original type of an argument. Both issues could be addressed by introducing a RISCVCCState (where I'd probably keep a Datalayout and a SubtargetInfo reference), but looking at GlobalISel that wouldn't actually help without further changes - CallLowering::handleAssignments creates a CCState directly, meaning you'd need to make further changes to support a custom CCState (as used in Hexagon, SystemZ, Mips and other backends).

I think some more GlobalISel design work needs to be done here really. It looks like splitting a smaller assignArgs function out of handleAssignments and making it virtual would help, but still wouldn't provide complete drop-in compatibility with custom CCState subclasses.

Hi Alex,

Thanks for your review!

My main concern with this approach is it will fail to work when VarArg support is added (D40805).

I will apply the patch of D40805 and read carefully about it.

meaning you'd need to make further changes to support a custom CCState (as used in Hexagon, SystemZ, Mips and other backends)

I will learn from other Targets to support a custom RISCVCCState.

I think some more GlobalISel design work needs to be done here really.

My sincere thanks will goto the developers who taught me a lot :) Thanks for your leading!

Regards,
Leslie Zhai

xiangzhai updated this revision to Diff 129404.Jan 11 2018, 1:34 AM

Hi LLVM developers,

Thanks for your code view, I can rebase my patch based on D40805 .

  1. Consider about IsFixed and OrigTy, but assert as firstly:
assert(IsFixed && "IsFixed support not yet implemented");
assert(OrigTy && "OrigTy support not yet implemented");

The implementation of RISCVCCState will be in the next patch, please give me some suggestion about custom RISCVCCState inherited from CCState, I think, at least, it needs to cover IsFixed and OrigTy.

  1. Remove parameter DL for CC_RISCV, because it is able to use State.getMachineFunction().getDataLayout() through parameter State.

Please review my patch again, thanks a lot!

Regards,
Leslie Zhai

asb added a comment.Jan 18 2018, 4:06 AM

Hi Leslie. Obviously we need to resolve the CCState issue before merging this, as it would regress codegen in its current state. Good observation that we can get the DataLayout via CCState. Given that, why not get rid of the various wrapper functions and just determine XLen and XLenVT from the DataLayout?

Hi Alex,

Thanks for your leading!

why not get rid of the various wrapper functions and just determine XLen and XLenVT from the DataLayout?

Good idea!

Obviously we need to resolve the CCState issue before merging this.

Sorry for my Chinese English :) is there some patch related to custom <Target>CCState, or someone began to implement RISCVCCState? if not, I will try to implement it.

Regards,
Leslie Zhai