This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add PowerPC rotate related builtins and emit target independent code for XL compatibility
ClosedPublic

Authored by NeHuang on Jun 22 2021, 1:51 PM.

Details

Summary

This patch is in a series of patches to provide builtins for compatibility
with the XL compiler. This patch adds the builtins and emit target independent
code for rotate related operations.

Diff Detail

Event Timeline

NeHuang created this revision.Jun 22 2021, 1:51 PM
NeHuang requested review of this revision.Jun 22 2021, 1:51 PM
qiucf added a subscriber: qiucf.Jun 23 2021, 2:26 AM

Please provide context of the patch (git diff -U999) :-)

NeHuang updated this revision to Diff 354082.Jun 23 2021, 2:39 PM

gentle ping.

nemanjai requested changes to this revision.Jul 13 2021, 7:55 AM
nemanjai added inline comments.
clang/include/clang/Sema/Sema.h
12559

I don't think these names adequately describe what the check is. We are not checking if PPC/PPC64 is a run of ones. In fact, the concept of a contiguous mask is in no way specific to PPC so we don't really need to differentiate this as a PPC-specific check.
Also, the convention seems to be to prefix all names with Sema. Let's not part with that convention.
Perhaps SemaValueIsRunOfOnes() or SemaArgIsRunOfOnes().

clang/lib/Sema/SemaChecking.cpp
3315

Isn't a zero technically a contiguous run of ones (of length zero)?

3406

Please describe in a comment why we need to ensure that the argument is a contiguous mask.

3407

We seem to populate the Result here. If we already have the Result (i.e. the constant value) can't we just simply check if that is a run of ones? Then we can greatly simplify CheckPPCisRunOfOnes() and CheckPPC64isRunOfOnes() (and we may not actually need two functions since Result will be an APSInt that knows how wide it is. We could presumably just have:
bool SemaValueIsRunOfOnes(const APSInt &Val, unsigned Width);

This revision now requires changes to proceed.Jul 13 2021, 7:55 AM
NeHuang updated this revision to Diff 358761.Jul 14 2021, 5:10 PM
NeHuang marked 4 inline comments as done.

Address review comments from Nemanja.

nemanjai accepted this revision.Jul 15 2021, 6:50 AM

LGTM other than a couple of nits.

clang/lib/CodeGen/CGBuiltin.cpp
15175

Please add a comment describing the emitted code. Something like:

// Rotate and insert under mask operation.
// __rlwimi(rs, is, shift, mask)
// rotl(rs, shift) & mask) | (is & ~mask)
15181

Nit: s/shift/Shift to conform to variable naming conventions. Here and elsewhere.

This revision is now accepted and ready to land.Jul 15 2021, 6:50 AM
This revision was landed with ongoing or failed builds.Jul 15 2021, 8:26 AM
This revision was automatically updated to reflect the committed changes.