[PATCH] D70758: [PowerPC][NFC] Rename record instructions to use _record suffix instead of o

Tue Dec 3 11:10:46 PST 2019

jsji marked an inline comment as done.
jsji added a comment.

In D70758#1766818 <https://reviews.llvm.org/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 for the kindness to support efforts for helping newcomers. @nemanjai

In D70758#1767084 <https://reviews.llvm.org/D70758#1767084>, @jhibbits wrote:

> I still hold pretty strongly that this change is a more negative impact than positive.

Can you be more specific about the negative impact of new proposal (_record suffix)?

What I can think of is:

1. longer name
2. breaking existing user's habit
3. breaking downstream code.

#3 shouldn't be a problem if we agree that it is a correct direction to make the code more readable and less confusion, to make it easier for newcomers to contribute.
#2 might be annoying to existing developers, especially experts like you and @nemanjai. However, I believe experts like you will have mercy on newcomers.
#1 I think it depends on how you look at it. -- To me, it is more verbose, and also call more attention to such opcodes: paying attention that this is a record form instruction, it is different from normal form!

> As shown in the tests, it's very easy to read the generated statements as "FOO-dot", and search for what 'foo.' actually is.

Again, it is easy for experts like you, but non obvious for newcomers without background.

> Searching the POWER ISA 3.0B reference, I see IBM using "period(.)", while the "EREF_RM" (NXP/Freescale Book-E reference) uses "dot suffix", with "record-form" only being used when describing the actual encoding.

Thanks for double check both Power ISA and EREF_RM. Yeah, so, `record-form` is the **ONLY** common terminology here.

If you still feel strongly against the new suffixes (_record) that Hal proposed, do you have any other suggestions?

Stepping back, or do you think it is NOT confusing at all? or you think it is somehow a little confusing but no good way to fix it?

Thanks. @jhibbits

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1370
   case PPCISD::UINT_VEC_TO_FP:  return "PPCISD::UINT_VEC_TO_FP";
-  case PPCISD::ANDIo_1_EQ_BIT:  return "PPCISD::ANDIo_1_EQ_BIT";
-  case PPCISD::ANDIo_1_GT_BIT:  return "PPCISD::ANDIo_1_GT_BIT";
+  case PPCISD::ANDI_record_1_EQ_BIT:  return "PPCISD::ANDI_record_1_EQ_BIT";
+  case PPCISD::ANDI_record_1_GT_BIT:  return "PPCISD::ANDI_record_1_GT_BIT";
----------------
nemanjai wrote:
> Ugh.
Agree that this ISD name is somehow ugly. We can definitely rename them if needed.
However, looks like to me that they are for a 'AND glue' bug only, should eventually go away.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70758/new/

https://reviews.llvm.org/D70758