This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix EmitPPCBuiltinExpr to emit arguments once
ClosedPublic

Authored by quinnp on Mar 14 2022, 1:44 PM.

Details

Summary

This patch changes EmitPPCBuiltinExpr in CGBuiltin.cpp to remove
the loop at the beginning of the function that emits the arguments and
to delay emitting the arguments until inside the switch statement. These
changes will put EmitPPCBuiltinExpr in line with the strategy of the
target independent function EmitBuiltinExpr. Also, this patch
ensures that arguments are only emitted once.

Tests that included builtins affected by these changes have been
modified to match expected behaviour.

Diff Detail

Event Timeline

quinnp created this revision.Mar 14 2022, 1:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 1:44 PM
quinnp requested review of this revision.Mar 14 2022, 1:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 1:44 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
quinnp added reviewers: Restricted Project, nemanjai, lei.Mar 14 2022, 1:45 PM
quinnp updated this revision to Diff 415412.Mar 15 2022, 6:29 AM

Minor formatting update.

quinnp edited the summary of this revision. (Show Details)Mar 15 2022, 7:45 AM
quinnp edited the summary of this revision. (Show Details)Mar 15 2022, 7:47 AM
quinnp updated this revision to Diff 416917.Mar 21 2022, 6:54 AM

Fixing a failing test case.

amyk added a subscriber: amyk.Mar 28 2022, 6:18 AM

Overall this change looks good to me, although I do have one question.

clang/lib/CodeGen/CGBuiltin.cpp
15287

A question I have is do we not need to consider this/EmitArrayToPointerDecay() anymore? Was this not used for anything?

quinnp added inline comments.Mar 28 2022, 1:03 PM
clang/lib/CodeGen/CGBuiltin.cpp
15287

Thanks for your comment! I could not find any builtins that used this case in the loop for emitting their arguments and did not see any failures when I removed it. I am going to look into this now and verify whether or not this is needed.

quinnp updated this revision to Diff 419126.Mar 30 2022, 6:22 AM

Moving testcases that require code generation from clang/test/Sema/ppc-pair-mma-types.c to clang/test/CodeGen/PowerPC/ppc-mma-types.c and clang/test/CodeGen/PowerPC/builtins-ppc-pair-mma-types.c. This is because some of these tests use the line Ops.push_back(EmitArrayToPointerDecay(E->getArg(i)).getPointer()); which I initially removed from EmitPPCbuiltinExpr. I have added this back in the switch statement where it is needed.

quinnp marked an inline comment as done.Mar 30 2022, 6:26 AM
quinnp added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
15287

I've updated the patch to include EmitArrayToPointerDecay() where it is needed and add code generation testcases to test this functionality.

amyk accepted this revision as: amyk.EditedApr 4 2022, 6:24 AM

Thanks Quinn. I think this overall LGTM.

This revision is now accepted and ready to land.Apr 4 2022, 6:24 AM
nemanjai accepted this revision.Apr 7 2022, 6:59 AM

Please add a test case that would cause a failure prior to this patch due to the argument being emitted more than once (i.e. the test case that prompted this patch). If that is already added and I just missed it, please add a note to the test case along the lines of:

// The argument expression must not be emitted multiple times.

Other than the minor nits, LGTM.

clang/lib/CodeGen/CGBuiltin.cpp
15718

Nit: I understand that we only have one use of E->getArg(1), but might as well initialize Op1 as above just for consistency.

quinnp updated this revision to Diff 421265.Apr 7 2022, 10:21 AM

Adressing review comments. Added a testcase which fails prior to this patch due to the arguments being emmited multiple times. Refactored some calls to EmitScalarExpr() to be more consistent.

quinnp updated this revision to Diff 421279.Apr 7 2022, 10:36 AM
quinnp marked an inline comment as done.

Fixing some testcases that broke due to re-ordering IR in my last update.

clang/lib/CodeGen/CGBuiltin.cpp
15718

I've fixed all the instances of this.

quinnp updated this revision to Diff 421286.Apr 7 2022, 10:51 AM

Rebasing with main.

quinnp updated this revision to Diff 421303.Apr 7 2022, 11:38 AM

Fixing a set of builtins added by the rebase with main.

This revision was landed with ongoing or failed builds.Apr 7 2022, 2:00 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 7 2022, 2:19 PM

Looks like this breaks tests on windows: http://45.33.8.238/win/55893/step_7.txt

Please take a look and revert for now if it takes a while to fix.

quinnp added a comment.Apr 7 2022, 2:49 PM

Looks like this breaks tests on windows: http://45.33.8.238/win/55893/step_7.txt

Please take a look and revert for now if it takes a while to fix.

Thanks for finding this, not sure what the cause is. I reverted the commit here: https://reviews.llvm.org/rGfef56f79ac8c4a4985774ea9fb1faa83a74866d3.

I'll look into a fix.

quinnp reopened this revision.Apr 12 2022, 11:53 AM

Re-opening the revision so that I can update it with a fix.

This revision is now accepted and ready to land.Apr 12 2022, 11:53 AM
quinnp updated this revision to Diff 422307.Apr 12 2022, 11:53 AM

Updating patch with a fix for emitting builtin arguments in an unspecified order.

quinnp updated this revision to Diff 422308.Apr 12 2022, 11:56 AM

Small change for consistency. llvm::Value -> Value

quinnp updated this revision to Diff 422312.Apr 12 2022, 12:17 PM

Small change for consistency.

quinnp updated this revision to Diff 422318.Apr 12 2022, 1:12 PM

Fixing failing tests added in this patch.

This revision was landed with ongoing or failed builds.Apr 13 2022, 2:32 PM
This revision was automatically updated to reflect the committed changes.