Page MenuHomePhabricator

[PowerPC] Replace cntlz[.] with cntlzw[.]
ClosedPublic

Authored by trixirt on Sep 24 2015, 11:07 AM.

Details

Reviewers
hfinkel
void
Summary

cntlz is the old POWER mnemonic.
cntlzw is the PowerPC mnemonic.

This change fixes an issue when -no-integrated-as.
The opcode cntlz is unrecognized by gas

Change PowerPC tests to reflect the insn change from cntlz to cntlzw.

Alias the POWER mnemonic cntlz[.] to the PowerPC mnemonic cntlzw[.]
This is done for because the POWER cntlz mnemonic has be used in llvm for
a very long time. We need to make sure that assembly programs
that are using the cntlz[.] do not break with this change.

Add assembly test to verify cntlz[.] is encoded correctly.

Diff Detail

Event Timeline

trixirt updated this revision to Diff 35656.Sep 24 2015, 11:07 AM
trixirt retitled this revision from to [PowerPC] Replace cntlz[.] with cntlzw[.].
trixirt updated this object.
trixirt added a reviewer: hfinkel.
trixirt added a subscriber: llvm-commits.
hfinkel edited edge metadata.Sep 25 2015, 5:44 AM

I think this is fine, although given that we've accepted the older form for a long time (and it was apparently in use, at least on some older Darwin systems), we should be kind to our users and continue to accept it as an alias. Please add the InstAlias defs and associated parsing tests (set InstAlias's Emit parameter to 0).

Also, as best I can tell from https://llvm.org/bugs/show_bug.cgi?id=1095 and http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20070115/042738.html, Darwin also wanted the newer mnemonic spelling. This is confusing, given that we have old tests specifically for the old form. Bill, if you can remember from that long ago, can you comment on what we should do here for Darwin?

[+David Fang; maybe you can comment on Darwin]

  • Original Message -----

From: hfinkel@anl.gov
To: tom@bumblecow.com, isanbard@gmail.com, hfinkel@anl.gov
Cc: llvm-commits@lists.llvm.org
Sent: Friday, September 25, 2015 7:44:12 AM
Subject: Re: [PATCH] D13142: [PowerPC] Replace cntlz[.] with cntlzw[.]

hfinkel added a comment.

I think this is fine, although given that we've accepted the older
form for a long time (and it was apparently in use, at least on some
older Darwin systems), we should be kind to our users and continue
to accept it as an alias. Please add the InstAlias defs and
associated parsing tests (set InstAlias's Emit parameter to 0).

Also, as best I can tell from
https://llvm.org/bugs/show_bug.cgi?id=1095 and
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20070115/042738.html,
Darwin also wanted the newer mnemonic spelling. This is confusing,
given that we have old tests specifically for the old form. Bill, if
you can remember from that long ago, can you comment on what we
should do here for Darwin?

http://reviews.llvm.org/D13142

void edited edge metadata.Sep 25 2015, 4:55 PM
void added a subscriber: void.

Unfortunately, I only vaguely recall this patch. Is there a reason not to
have support for both? I don't think we support POWER in LLVM though (I
could be wrong there) ... You may need to ask one of the ppc experts here.

trixirt updated this revision to Diff 35821.Sep 27 2015, 9:48 AM
trixirt updated this object.
trixirt edited edge metadata.

Added alias back to old cntlz mnemonic.
So old, stand alone assembly programs will still work.

The code generation will only use the PowerPC mnemonic.
No obvious flag to hang the POWER variant off of.

A non llvm program using the generated assembly would break if it only handled the
POWER variant.

hfinkel accepted this revision.Sep 28 2015, 1:09 AM
hfinkel edited edge metadata.

LGTM.

lib/Target/PowerPC/PPCInstrInfo.td
3888

I'd specifically mention in the comment that the reason we're supporting this particular POWER alias is because LLVM used only this form for many years.

This revision is now accepted and ready to land.Sep 28 2015, 1:09 AM
trixirt updated this revision to Diff 36470.Oct 4 2015, 9:19 AM
trixirt updated this object.
trixirt edited edge metadata.

improve commit log to explain long use of cntlz is why this POWER mnemonic is supported.

Cleanup whitespace problems in tests.

hfinkel closed this revision.Oct 27 2015, 8:29 PM

r251489.