[PATCH] D23630: [PPC] Expand ISEL instruction into if-then-else sequence

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:85
+  ///
+  /// Expand ISEL instructions into if-the-else sequence on POWER7 and POWER8.
+  /// Generate ISEL instruction on other architectures.
----------------
This is just not true. Please see the comment below at the definition of the function.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:129
+
+bool PPCExpandISEL::isExpandISELEnabled(const MachineFunction &MF) {
+  if (GenerateISEL)
----------------
At this point, I would question the applicability of this function. We now have an option that is meant to tell the compiler to generate or not generate the instruction, but has unexpected behaviour on some platforms.
Namely, when the option is specified to be `true`, the compiler will generate the isel on every platform. On the other hand, when the option is specified to be `false`, the compiler will not generate the isel on 64-bit non-Darwin targets. This of course, may be the desired behaviour (i.e. always generate isel on 32-bit or Darwin targets), but it should be documented as such.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:166
+
+bool PPCExpandISEL::expandISEL() {
+  for (ISELInstructionList::iterator BlockList = ISELInstructions.begin(),
----------------
I am not in favour of naming this function the same as the other overload. The two overloads do different things - this one expands ALL isel's, the other overload expands only ONE isel. I know this is somewhat common, but I think giving this one a name that suggests this plurality aids readability of the code.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:176
+      if (!expandISEL(**I)) {
+        DEBUG(dbgs() << "Unable to expand isel for " << I << "\n");
+        return false;
----------------
I assume you have tested this, but I am just curious if dumping this iterator without dereferencing it prints the actual MI.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:195
+
+  bool isTrueBlockRequired = (Dest.getReg() != TrueValue.getReg());
+  bool isFalseBlockRequired = (Dest.getReg() != FalseValue.getReg());
----------------
These names don't conform to the naming convention (variables start with a capital letter).


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:199
+         "True and False values equivalent in isel instruction!\n");
+
+  DEBUG(dbgs() << "Dest: " << Dest << "\n");
----------------
This is a very long function. I wonder if it would be a little easier to follow the code if it was split into (for example):
```
PPCExpandISEL::expandISELBothBlocks()
PPCExpandISEL::expandISELNoTrueBlock()
PPCExpandISEL::expandISELNoFalseBlock()
```

Or if that would lead to code duplication, perhaps just move all the code that sets up the basic block layout as needed in the given situation into
`PPCExpandISEL::reorganizeBlockLayout()`


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:215
+  MachineBasicBlock *NewSuccessor = MBBI != MBB->getLastNonDebugInstr()
+                                       ? Fn->CreateMachineBasicBlock(LLVM_BB)
+                                       : nullptr;
----------------
The indentation seems off here.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:244
+
+  if (NewSuccessor) {
+    Fn->insert(It, NewSuccessor);
----------------
I think this should be moved up to where you select how to set `Successor`. Basically within that conditional statement, handle reordering the layout of the basic blocks and then move on to creating the true/false blocks as necessary.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:272
+
+  if (isTrueBlockRequired) {
+    TrueBlockI = TrueBlock->begin();
----------------
I don't understand the need for this strange code layout. Namely, you have:
```if (isTrueBlockRequired)
  // do true block stuff
if (isFalseBlockRequired)
  // do false block stuff
if (isTrueBlockRequired)
  // do more true block stuff
if (isFalseBlockRequired)
  // do more false block stuff
...
```
Why can't the code in these conditional blocks be collapsed together?


https://reviews.llvm.org/D23630