Page MenuHomePhabricator

[RFC][Tablegen] Add CCIfSplitFrom and CCPassIndirectBySamePointer Calling Convention Interfaces
Needs ReviewPublic

Authored by shiva0217 on Oct 13 2017, 9:44 AM.

Details

Reviewers
rnk
stoklund
Summary

Hi,

For the target ABI have following calling convention

"Pass by indirect pointer if the primary type is f128"

If the target didn't support f128 register class, legalizer will split the arguments.
So it's hard to identify the argument is split from f128 by *CallingConv.td,
and we have to obtain custom calling convention handler to deal with.

To pass the split parts by the same indirect pointer would also need extra handler function.
One of the cases is "CC_SystemZ_I128Indirect" in SystemZCallingConv.h.

To simplify the porting effort, I'd like to add the interfaces as follow.

CCIfSplitFrom - If the current argument is split from one of the specified types, apply action A.

CCPassIndirectBySamePointer - Same as CCPassIndirect except pass by the same pointer if the arguments have been split by legalizer.

With the new calling convention interfaces, we could describe

"Pass by indirect pointer if primary type is f128" as

CCIfType<[i64], CCIfSplitFrom<[f128], CCPassIndirectBySamePointer<i64>>>

without writing custom handling function by CCCustom.

The Tablegen result will be

  if (LocVT == MVT::i64) {
        if (ArgFlags.getOrigVt() == MVT::f128) {
	      LocVT = MVT::i64;
	      LocInfo = CCValAssign::Indirect;
	      if (!ArgFlags.isSplit()) {
	             State.addLoc(State.lastLoc());
	             return false;
              }
         }
   }

Any suggestions?

Thanks,
Shiva

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Oct 13 2017, 9:44 AM
rnk requested changes to this revision.Oct 13 2017, 9:46 AM
rnk added a subscriber: rnk.

This needs tests, and it would surely be simpler if the frontend didn't emit illegal types as arguments. We normally handle this kind of thing in clang, so you would get an LLVM f128* argument.

This revision now requires changes to proceed.Oct 13 2017, 9:46 AM
In D38894#897090, @rnk wrote:

This needs tests, and it would surely be simpler if the frontend didn't emit illegal types as arguments. We normally handle this kind of thing in clang, so you would get an LLVM f128* argument.

Hi @rnk, Thanks for the quick reply!
Yes, most of the cases we could handle by clang.
One of the cases couldn't is that the function created by llvm. E.g. softfloat functions which clang will see it as floating operations.
So we still need to write a custom function to deal with.

Could you guide me how to add the test case for the calling convention interfaces?
Should I patch some backend and write MC testcases for the backend or there is better way ?

shiva0217 edited edge metadata.
shiva0217 added a subscriber: uweigand.

Hi @rnk,

I have updated SystemZCallingConv.td to use new interfaces for i128 type arguments.
SystemZ already have llvm/test/CodeGen/SystemZ/args-09.ll
for testing i128 type arguements passing.

Add SystemZ Owner @uweigand for reviewing

The SystemZ parts LGTM. I agree that it's much preferable to have this handled in common code than in the back end ...

rnk added a comment.Oct 24 2017, 7:51 AM
In D38894#897090, @rnk wrote:

This needs tests, and it would surely be simpler if the frontend didn't emit illegal types as arguments. We normally handle this kind of thing in clang, so you would get an LLVM f128* argument.

Hi @rnk, Thanks for the quick reply!
Yes, most of the cases we could handle by clang.
One of the cases couldn't is that the function created by llvm. E.g. softfloat functions which clang will see it as floating operations.
So we still need to write a custom function to deal with.

OK, that sounds like a great test case then. :) Seeing it will help clear up why this is necessary. I'm still not convinced that it is.

Could you guide me how to add the test case for the calling convention interfaces?
Should I patch some backend and write MC testcases for the backend or there is better way ?

Get clang to emit a .ll file that needs this handling and add it to tests/CodeGen/SystemZ.

Well, the special handling of i128 arguments as such is certainly needed on SystemZ, this is why I added it in the first place in rev. 261325.

This was triggered by a bug report here: https://bugs.llvm.org/show_bug.cgi?id=26559 which in fact has C source code that clang translates to IR that exposes the problem.

Now this patch does not fix any problem in SystemZ in itself, it just moves the required handling from the back end to common code, making life easier for other back ends that need the same handling.

Hi @rnk and @uweigand,
The bug report:https://bugs.llvm.org/show_bug.cgi?id=26559 which @uweigand mentioned is the case that softfloat function created by llvm which clang won't handle it.

In the bug report, clang will emit "uitofp i128 %0 to float" floating operation.
LLVM will soften the floating operation to softfloat function call.

So I add the bug report case as args-11.ll as @rnk suggestion.

The patch is for making backend life easier. RISCV32 have the similar ABI and could have benefit with it.

This comment was removed by shiva0217.

Hi @rnk ,
In the test case, clang doesn't know backend support the floating instruction or not, so clang will emit it as floating operation IR and won't treat it as function call.
LLVM know the backend not support the floating operation, so it would soften to softfloat function call.
It was transferred to function call by llvm, therefore, in this case, clang can't help for transfer the arguments passing to indirect.
Could the test case convinced you the patch could be helpful?
Or should I add/modify something to make the patch more complete/correct?

Thanks,
Shiva

asb added a subscriber: asb.Nov 9 2017, 12:30 PM
asb added inline comments.Nov 9 2017, 12:39 PM
include/llvm/Target/TargetCallingConv.td
160

Can you make this comment clearer? i.e. explain that the difference in behaviour is when this is applied to split arguments.

shiva0217 updated this revision to Diff 122392.Nov 9 2017, 10:06 PM
shiva0217 edited the summary of this revision. (Show Details)

Update comments of CCPassIndirectBySamePointer as @asb suggest.
Make it clear that the differences between CCPassIndirectBySamePointer and CCPassIndirect.
CCPassIndirectBySamePointer will assign the same pointer if the arguments have been split by legalizer.

mgrang added a subscriber: mgrang.Nov 17 2017, 10:59 AM
mgrang added inline comments.
include/llvm/CodeGen/CallingConvLower.h
263

Can you leave out the void from the empty parameter type?

test/CodeGen/SystemZ/args-11.ll
17

Check indentation.

shiva0217 updated this revision to Diff 123522.Nov 19 2017, 5:22 PM
shiva0217 edited the summary of this revision. (Show Details)
shiva0217 set the repository for this revision to rL LLVM.
shiva0217 updated this revision to Diff 123523.Nov 19 2017, 5:30 PM
shiva0217 added inline comments.Nov 19 2017, 5:32 PM
include/llvm/CodeGen/CallingConvLower.h
263

Remove void from empty parameter type as @mgrang suggest.

test/CodeGen/SystemZ/args-11.ll
17

Fix indentation.

apazos added a subscriber: apazos.Nov 20 2017, 2:08 PM
apazos added inline comments.
include/llvm/Target/TargetCallingConv.td
35

If the current argument is splited -->is split

shiva0217 updated this revision to Diff 123696.Nov 20 2017, 5:19 PM
shiva0217 removed a subscriber: apazos.
shiva0217 added inline comments.
include/llvm/Target/TargetCallingConv.td
35

Fixup comment.

I think I accidentally remove one subscriber, sorry about that, add back subscriber.