Page MenuHomePhabricator

[NVPTX] Check if callsite is defined when computing argument allignment
ClosedPublic

Authored by jpienaar on Apr 21 2015, 12:52 PM.

Details

Summary

In getArgumentAlignment check if the ImmutableCallSite pointer CS is non-null before dereferencing. If CS is 0x0 fall back to the ABI type alignment else compute the alignment as before.

Diff Detail

Event Timeline

jpienaar updated this revision to Diff 24160.Apr 21 2015, 12:52 PM
jpienaar retitled this revision from to [NVPTX] Check if callsite is defined when computing argument allignment.
jpienaar updated this object.
jpienaar edited the test plan for this revision. (Show Details)
jpienaar added reviewers: jingyue, eliben.
jpienaar added a subscriber: Unknown Object (MLST).
jingyue edited edge metadata.Apr 21 2015, 12:57 PM

test? :)

jingyue resigned from this revision.Mar 15 2016, 10:30 AM
jingyue removed a reviewer: jingyue.

I have a potential test-case for this, but the patch doesn't apply cleanly to master so I was unable to test if this solves the problem.

jpienaar abandoned this revision.Sep 19 2016, 9:31 AM

Oh, sorry, I didn't see your response before I clicked abandoned. It has been a while, so this patch is pretty stale.

@jpienaar are you planning to work on this again? Or should I give it a go?

vchuravy updated this revision to Diff 71936.Sep 20 2016, 8:11 AM

Rebases the original changes and adds a test-case

This is my first contribution to llvm so please let me know if I did something wrong in the process

Cool. I didn't know the review system allows having the patch updated like this :) It still reports me as the author and you as a subscriber. I don't think that matters.

lib/Target/NVPTX/NVPTXISelLowering.cpp
1051

There is a preference to early exits. So perhaps:

if (!CS)
  return DL.getABITTypeAlignment(Ty);
1132–1133

Move this to a new line to avoid exceeding 80 chars.

There are a couple of other formatting changes needed to. The simplest way is to use clang-format. To have the changes you added reformatted you could use clang-format-diff.py:
svn diff --diff-cmd=diff -x-U0 | ./tools/clang/tools/clang-format/clang-format-diff.py

test/CodeGen/NVPTX/zero-cs.ll
2

Could you add a comment explaining what this test is testing?

So this test case would fail previously (dereference null pointer) and now pass (return error)?

vchuravy updated this revision to Diff 71946.Sep 20 2016, 8:58 AM

addresses review comments.

You don't own revision D9168: "[NVPTX] Check if callsite is defined when
computing argument allignment". Normally, you should only update
revisions you own. You can "Commandeer" this revision from the web
interface if you want to become the owner.

I was not able to figure out how to comandeer a revision, so i just went ahead and pushed it.

jpienaar accepted this revision.Sep 20 2016, 9:22 AM
jpienaar added a reviewer: jpienaar.

Looks good to me, do you need help submitting?

lib/Target/NVPTX/NVPTXISelLowering.cpp
1032

s/callsize/Call site/ ?

This revision is now accepted and ready to land.Sep 20 2016, 9:22 AM
vchuravy updated this revision to Diff 71951.Sep 20 2016, 10:20 AM
vchuravy edited edge metadata.

Fix spelling in comment

@jpienaar Since I don't have commit access, I think somebody else needs to commit this.

jlebar added a subscriber: jlebar.Sep 20 2016, 6:57 PM

FWIW I have run into this in the past and just not managed to muster up the energy to fix it. So, thank you!

I was not able to figure out how to comandeer a revision, so i just went ahead and pushed it.

Under "leap into action", one of the options is to commandeer the revision.

jpienaar closed this revision.Sep 20 2016, 7:06 PM