Page MenuHomePhabricator

[RFC] Add IsFixed field to ISD::ArgFlagsTy
Needs ReviewPublic

Authored by asb on Jan 22 2018, 7:25 AM.

Details

Summary

As discussed on llvm-dev, it's unfortunate that ISD::ArgFlagsTy doesn't contain IsFixed. Adding this field isn't enough to get rid of SystemZ, PPC or Mips custom CCState, but does allow simplifications in the Lanai and Mips backends as well as removal of HexagonCCState.

This patch doesn't update any backend's GlobalISel call lowering to use the new field. Further improvements are possible: AArch64ISelLowering::CCAssignFnForCall could be updated to not take an IsVarArg parameter, AArch64 vararg calling conventions could be removed and CCIfFixed used instead, and CCAssignFnForCall could then be called only once per function rather than once per argument.

Posted with the hope of getting initial feedback on this direction.

Diff Detail

Event Timeline

asb created this revision.Jan 22 2018, 7:25 AM
asb added a subscriber: kparzysz.Jan 22 2018, 7:25 AM
dsanders added inline comments.Jan 22 2018, 10:44 AM
include/llvm/CodeGen/GlobalISel/CallLowering.h
53–54

It looks like SelectionDAG and FastISel are both calling Flags.setFixed() themselves. Do we still need the IsFixed argument or is GlobalISel still using it?

asb added inline comments.Jan 23 2018, 2:03 AM
include/llvm/CodeGen/GlobalISel/CallLowering.h
53–54

The IsFixed field could fairly easily be removed. Ideally setArgFlags would also set IsFixed (probably by adding a bool IsFixed arg to that method), though I'd need to audit current GlobalISel to make sure setArgFlags is _always_ called. It seems it's not currently called for void return types at least.

Simply changing assignArg in AArch64CallLowering.cpp to use Info.Flags.isFixed() rather than Info.IsFixed seems to result in some minor test output changes. Given that Flags.IsFixed always set at the time the ArgInfo struct is created, there's clearly something I'm missing about the lifetime of Flags...

chenwj added a subscriber: chenwj.Jan 23 2018, 5:12 AM

I think this field is useful for most targets. Just a little concern about the coding style.

lib/CodeGen/SelectionDAG/FastISel.cpp
976

Would changing the range-based for loop to index-based one be more consistent?

asb added inline comments.Jan 23 2018, 5:20 AM
lib/CodeGen/SelectionDAG/FastISel.cpp
976

Yes I think that would be better. I put this together quickly to get initial feedback on the overall direction. I'll move to an index-based loop in the next update. Thanks for taking a look!

Hi Alex,
I think it's great that the target description file could determine the argument is fixed or not. In this way, the targets have different ABI behavior for non-fixed arguments could describe by TD files without extra custom c++ codes. It seems that Mips and RISCV already have variable argument test cases to cover the changed. Do other targets involve the changed have variable argument test case? If it's not, could you add the test cases to cover the changed?