Page MenuHomePhabricator

[M68k][GloballSel] Formal arguments lowering in IRTranslator
ClosedPublic

Authored by sushmaunnibhavi on Jun 18 2021, 8:46 AM.

Diff Detail

Event Timeline

sushmaunnibhavi requested review of this revision.Jun 18 2021, 8:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2021, 8:46 AM
sushmaunnibhavi edited the summary of this revision. (Show Details)Jun 18 2021, 8:49 AM
sushmaunnibhavi retitled this revision from [M68k][GloballSel]Lower i32, i32 values to [M68k][GloballSel] Lower i32, i32 values.
sushmaunnibhavi edited the summary of this revision. (Show Details)
gandhi21299 added inline comments.Jun 19 2021, 10:14 AM
llvm/test/CodeGen/M68k/GlobalISel/irtranslator-ret.ll
12

Should add checks for the parameters

gandhi21299 added inline comments.Jun 19 2021, 10:30 AM
llvm/test/CodeGen/M68k/GlobalISel/irtranslator-ret.ll
178

add more tests with parameters of various types: aggregate types, non-aggregate types, combination of those

Title of this patch is not clear enough, perhaps something like [M68k] [GlobalISel] Formal arguments lowering in IRTranslator.

Also, I just realized this is only for i32 values so far. I think it should be easily extensible across any primitive type. Thoughts @myhsu ?

myhsu added a comment.Jun 20 2021, 5:45 PM

Title of this patch is not clear enough, perhaps something like [M68k] [GlobalISel] Formal arguments lowering in IRTranslator.

Also, I just realized this is only for i32 values so far. I think it should be easily extensible across any primitive type. Thoughts @myhsu ?

Agree, please try to augment this patch into generic integer type

myhsu added inline comments.Jun 20 2021, 5:51 PM
llvm/lib/Target/M68k/GlSel/M68kCallLowering.cpp
82

I think you're trying to say either "Value should be assigned to reg" or "Value was not assigned to reg?" right?

97

You can just put "unimeplemented" here. Since the stack trace will tell you which function triggers this unreachable statement

104

ditto

llvm/lib/Target/M68k/GlSel/M68kCallLowering.h
70

(nit) please use struct instead and remove "public" in the following line

llvm/lib/Target/M68k/M68kISelLowering.h
174

The function name is a little confusing, maybe something like "getCCAssignGnForCall"?

sushmaunnibhavi updated this revision to Diff 353886.EditedJun 23 2021, 1:51 AM
sushmaunnibhavi retitled this revision from [M68k][GloballSel] Lower i32, i32 values to [M68k][GloballSel] Lower two integer values.
sushmaunnibhavi edited the summary of this revision. (Show Details)

Updated patch according to @myhsu and @gandhi21299 comments

sushmaunnibhavi marked 5 inline comments as done.Jun 23 2021, 1:52 AM
sushmaunnibhavi retitled this revision from [M68k][GloballSel] Lower two integer values to [M68k][GloballSel] Formal arguments lowering in IRTranslator.Jun 23 2021, 7:29 AM
gandhi21299 added inline comments.Jun 23 2021, 7:25 PM
llvm/test/CodeGen/M68k/GlobalISel/irtranslator-ret.ll
39

Shorter variable name instead of G_FRAME_INDEX, replaces uses and should not be reused for a different register

sushmaunnibhavi edited the summary of this revision. (Show Details)

Added tests for pointer, array and struct

sushmaunnibhavi marked an inline comment as done.Jun 24 2021, 4:19 AM
sushmaunnibhavi marked an inline comment as done.

LGTM, please wait for approval from at least one more reviewer before we merge this patch in. Great work @sushmaunnibhavi !

LGTM, please wait for approval from at least one more reviewer before we merge this patch in. Great work @sushmaunnibhavi !

Thanks for reviewing the patch @gandhi21299. @myhsu does the patch look good?

I'm curious if the test pass on your side. When I applied this patch on tip-of-tree, the newly-added test case triggered unreachable statement on line 87 of lib/Target/M68k/GlSel/M68kCallLowering.cpp (i.e. M68kIncomingValueHandler::getStackAddress was called).
Can you double check if this patch works on tip-of-tree? (of course please make sure you have assertion enabled in your build)

llvm/lib/Target/M68k/GlSel/M68kCallLowering.h
68

please use struct instead and remove "public" in this line as well as the empty line above

I'm curious if the test pass on your side. When I applied this patch on tip-of-tree, the newly-added test case triggered unreachable statement on line 87 of lib/Target/M68k/GlSel/M68kCallLowering.cpp (i.e. M68kIncomingValueHandler::getStackAddress was called).
Can you double check if this patch works on tip-of-tree? (of course please make sure you have assertion enabled in your build)

I did build my patch again but I don't get any errors.

myhsu added a comment.Jun 26 2021, 5:03 PM

I'm curious if the test pass on your side. When I applied this patch on tip-of-tree, the newly-added test case triggered unreachable statement on line 87 of lib/Target/M68k/GlSel/M68kCallLowering.cpp (i.e. M68kIncomingValueHandler::getStackAddress was called).
Can you double check if this patch works on tip-of-tree? (of course please make sure you have assertion enabled in your build)

I did build my patch again but I don't get any errors.

no, I didn't say it fail the build, I said it failed the test after this patch was applied.
In other words, ninja check-llvm-codegen-m68k fails (or replace ninja with whatever build system you're using). Can you verify whether that command fails on your side?

myhsu added a comment.EditedJun 26 2021, 5:24 PM

Also, since M68k's calling convention uses stack to pass incoming parameters, I'm not surprised M68kIncomingValueHandler::getStackAddress was called by GlobalISel to generate proper MIR instructions.
I suggest you to use i386 (a.k.a 32-bit X86) as a reference to see how thing works there and how they implement IncomingValueHandler. Since i386 is also using stack to pass incoming parameters.

sushmaunnibhavi added a comment.EditedJun 26 2021, 9:24 PM

I'm curious if the test pass on your side. When I applied this patch on tip-of-tree, the newly-added test case triggered unreachable statement on line 87 of lib/Target/M68k/GlSel/M68kCallLowering.cpp (i.e. M68kIncomingValueHandler::getStackAddress was called).
Can you double check if this patch works on tip-of-tree? (of course please make sure you have assertion enabled in your build)

I did build my patch again but I don't get any errors.

no, I didn't say it fail the build, I said it failed the test after this patch was applied.
In other words, ninja check-llvm-codegen-m68k fails (or replace ninja with whatever build system you're using). Can you verify whether that command fails on your side?

Yes I did this too and the tests didn't fail.

Also, since M68k's calling convention uses stack to pass incoming parameters, I'm not surprised M68kIncomingValueHandler::getStackAddress was called by GlobalISel to generate proper MIR instructions.
I suggest you to use i386 (a.k.a 32-bit X86) as a reference to see how thing works there and how they implement IncomingValueHandler. Since i386 is also using stack to pass incoming parameters.

I will look into this. Thanks!

myhsu added a comment.Jun 26 2021, 9:57 PM

I'm curious if the test pass on your side. When I applied this patch on tip-of-tree, the newly-added test case triggered unreachable statement on line 87 of lib/Target/M68k/GlSel/M68kCallLowering.cpp (i.e. M68kIncomingValueHandler::getStackAddress was called).
Can you double check if this patch works on tip-of-tree? (of course please make sure you have assertion enabled in your build)

I did build my patch again but I don't get any errors.

no, I didn't say it fail the build, I said it failed the test after this patch was applied.
In other words, ninja check-llvm-codegen-m68k fails (or replace ninja with whatever build system you're using). Can you verify whether that command fails on your side?

Yes I did this too and the tests didn't fail.

Hmmm...alright, let's see how the buildbot will say

llvm/lib/Target/M68k/GlSel/M68kCallLowering.h
68

still outstanding

Changed class to struct in M68CallLowering.h file

sushmaunnibhavi marked 2 inline comments as done.Jun 26 2021, 10:16 PM
myhsu accepted this revision.Jun 27 2021, 10:43 AM

LGTM

This revision is now accepted and ready to land.Jun 27 2021, 10:43 AM

Do you want me to commit on your behalf?

Do you want me to commit on your behalf?

Yes, because I don't have commit access.

This revision was landed with ongoing or failed builds.Jun 27 2021, 4:14 PM
This revision was automatically updated to reflect the committed changes.
myhsu added a comment.EditedJun 27 2021, 11:21 PM

I'm afraid the build fails: https://lab.llvm.org/staging/#/builders/180/builds/132
More specifically, here is the error details on the test case added by this patch: https://lab.llvm.org/staging/#/builders/180/builds/132/steps/5/logs/FAIL__LLVM__irtranslator-ret_ll
Which is identical to the assertion failure I pointed out earlier in this review.

@sushmaunnibhavi I'm afraid I need to temporarily revert the commit (since it is the only test case relates to the patch, it doesn't make sense to only mark the test XFAIL)

Here are my guesses of why you couldn't reproduce the failure:

  1. Your local changes are not sync with the patch presented here.
  2. You didn't turn on assertion. Please open <your build folder>/CMakeCache.txt. If the CMAKE_BUILD_TYPE variable is "Debug" then you're fine, if it's something else, make sure LLVM_ENABLE_ASSERTIONS is ON (in case you don't know, you can directly change the variable value in that file and CMake will reconfigure on the next build).

Regarding the root cause of the crash, M68k's calling convention relies on pushing parameters to the stack, so only implementing register assignment is not enough. As I suggested before, i386's implementation of GlobalISel might be a good reference.

Last but not the least, commits got reverted all the time in LLVM so please don't mind :-) You're almost there and I definitely appreciate your contribution here!

Here are my guesses of why you couldn't reproduce the failure:

  1. Your local changes are not sync with the patch presented here.
  2. You didn't turn on assertion. Please open <your build folder>/CMakeCache.txt. If the CMAKE_BUILD_TYPE variable is "Debug" then you're fine, if it's something else, make sure LLVM_ENABLE_ASSERTIONS is ON (in case you don't know, you can directly change the variable value in that file and CMake will reconfigure on the next build).

Regarding the root cause of the crash, M68k's calling convention relies on pushing parameters to the stack, so only implementing register assignment is not enough. As I suggested before, i386's implementation of GlobalISel might be a good reference.

Last but not the least, commits got reverted all the time in LLVM so please don't mind :-) You're almost there and I definitely appreciate your contribution here!

Thanks! I will look into this and fix it.

Added implementation for M68kIncomingValueHandler::getStackAddress.

sushmaunnibhavi reopened this revision.Jun 28 2021, 9:24 PM
This revision is now accepted and ready to land.Jun 28 2021, 9:24 PM

@myhsu can you check the patch now? All test cases pass on my side.

myhsu added inline comments.Jun 30 2021, 9:09 AM
llvm/lib/Target/M68k/GlSel/M68kCallLowering.cpp
81

Is there any specific reason to extract these code into a separate lambda if there is only one callsite

llvm/test/CodeGen/M68k/GlobalISel/irtranslator-ret.ll
15

Git told me that there are trailing white spaces after every G_FRAME_INDEX line in this file, please remove them

myhsu added a comment.Jun 30 2021, 9:11 AM

Basically looks good to me now. Please address the minor issue I point out in the inlined comments.

llvm/lib/Target/M68k/GlSel/M68kCallLowering.cpp
81

Yeah, this isn't necessary.

sushmaunnibhavi updated this revision to Diff 355618.EditedJun 30 2021, 10:35 AM

Removed trailing whitespaces after G_FRAME_INDEX

sushmaunnibhavi marked 2 inline comments as done.Jun 30 2021, 10:36 AM
myhsu accepted this revision.Jun 30 2021, 4:17 PM

LGTM Thank you!
I'll commit it later today

This revision was landed with ongoing or failed builds.Jun 30 2021, 5:14 PM
This revision was automatically updated to reflect the committed changes.