FYI. Needs rebase to pick up https://reviews.llvm.org/D72649. Thanks.

Ping...

In D72800#1823492, @foad wrote:

This patch would only change behaviour if the target's shouldClusterMemOps(SUa, SUb) might return a different answer from shouldClusterMemOps(SUb, SUa).

Thanks for review and accepting.

In D72649#1818217, @nemanjai wrote:

This introduces a dependency for https://reviews.llvm.org/D72569.

In D70758#1805850, @jhibbits wrote:

Not happy, but won't hold it up.

I'd rather see just the "isDOT" changed to "isRecord" and leave the rest as-is, but I'll defer to everyone else.

@jhibbits: are you OK with this now? Most of the other reviewers are OK with this now. Thanks.

Yes, in theory, it might affect scheduling and other opts as well.
Can we please prove it by providing a test case that has changes due to this patch. Thanks.

Rebase to show changes related to this change only.

LGTM.
Yes, it would be better if you can run update_llc_test_checks.py without this patch first, commit it, rebase, then run update_llc_test_checks.py with this patch again.

Handle fp128 and fp16.

clang-tidy: fail. Please fix clang-tidy findings.

clang-tidy failure fixed in https://reviews.llvm.org/rGfcbf05bbdccc8a32f6a80316ea1c13be7e7eeae2.

Rebase

I checked all references to isPredicable in codebase:

• Instr doc generation uses the bit (this is non-functional)
• CodeGen tablegen itself uses it
• Both TargetInstrInfo::isUnpredicatedTerminator and TargetInstrInfo::PredicateInstruction references the method, but PPC overrides them and doesn't use it.
• MachineInstr::findFirstPredOperandIdx references it, but only ARM and AMDGPU invokes it
• Implicit null check and machine sink have referenced it. However, this patch won't touch instrs may load or store, so result of expression would never change
• Other references are for other platforms

Thanks, Hal & Kai.
@jhibbits @nemanjai , are you OK with this now?

Looks like to me that Vector Compare in SPE also has different CR bit semantics .
So this patch will only handle floating point part for SPE? If so, maybe we should make it explicit in title.

Ping ...

If this is a NFC change, please add [NFC] in summary. Or else, please add test. Thanks.

@Jim Are you intend to block this by requesting changes? If so, can you have a look again? Thanks.

LGTM.

Ping....

Ping...

@ZhangKang Can we get some performance data for this change. Thanks.

@steven.zhang can you work with @ZhangKang to see why, and whether we rely on hasSideEffects for a better schedule right now. Thanks.

Rename to _rec instead.

In D70758#1776123, @hfinkel wrote:

So the common subset is _rec, right?

Thanks. I will commit it for you.

Hi all, thanks for the review and discussion.
I'd like to make a final decision about commit or abandon, I don't see any point of leaving this longer.

@lkail When committing code for others, please follow the format described in Developer Policy. Thanks.
https://llvm.org/docs/DeveloperPolicy.html#attribution-of-changes

In D70758#1771145, @jhibbits wrote:

I might object less to a _rec suffix. However, you would still need to understand what "record form" means, which requires understanding of the ISA. Why not simply add a comment block to the top of PPCInstrInfo.td, describing things like this? I have a feeling other newbies would have similar questions/concerns/complaints, and they can't all be addressed by renaming everything.

Rebased.

In D70758#1767545, @jhibbits wrote:

I personally think the status quo is easy to read. The definitions look like the instructions themselves, and the added verbosity seems very unnecessary. I don't know about others, but I consider people working in the PPCInstr*.td files to be either knowing exactly what they're looking for (grepping), or have a sufficient handle on the ISA that the existing notation is clear. At most I could see changing the 'o' suffix to '_o', which might help newcomers familiar with '_' notation to indicate a diminutive or subscript, but '_record' is very verbose, and as @nemanjai points out, can make some text very ugly.

In D70758#1766818, @nemanjai wrote:

So since the status quo is at the top of my list of naming convention preferences in this case, I will abstain from further comments and will accept whatever the majority decides.

Thanks @jhibbits !

Rename RLWINM(I)_recordbm to RLWINM(I)bm_record

Rename record instructions to use _record suffix instead of o
Rename isDot to isRecordForm

To make things more obvious, we could replace isDOT with isRecordForm, the 'o' suffix with _record (moving the numeric suffix, where present, to before the underscore). What do you think of that?

In D70758#1766055, @jhibbits wrote:

When discussing optimizations related to "ADD-o", if it's "ADDO" I'd call it "ADD-O", but if it's "ADDo" I'd call it "ADD-dot",

In D70758#1765982, @jhibbits wrote:

I second @nemanjai, the lowercase 'o' suffix stands out to me as a clear indicator of it being punctuation, not a letter in the instruction. I would much rather see ANDIo than ANDIr, the 'r' just doesn't look right when reading over it. My vote is to reject. It looks like a lot of churn without much benefit.

Furthermore, the flag we set in the multiclasses is called isDOT and there is no clear relationship between this "dot" and the letter r. All that said, I will not block this if the majority is in agreement (or indifferent). But let's get some more eyes on this.

Rebased to resolve conflict.