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

[Kit Barton via llvm-commits]

kbarton requested changes to this revision.
kbarton added a comment.
This revision now requires changes to proceed.

There are some things that need to get cleaned up.



================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:34
+                 cl::desc("Enable generating the ISEL instruction"),
+                 cl::Hidden);
+
----------------
nemanjai wrote:
> I think it's clearer if you explicitly initialize this to false (presumably that's the default).
Good idea. I will change this. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:129
+
+  if (MF.getSubtarget<PPCSubtarget>().isSVR4ABI() &&
+      MF.getSubtarget<PPCSubtarget>().isPPC64())
----------------
nemanjai wrote:
> The doxygen comment for this function states that we want to expand the isel instructions on Power7 and Power8 processors but this actually relies on different conditions. I am not entirely clear on whether the SVR4 ABI only applies on those processors, but the conditions seem to be mismatched.
These checks are really messy right now, in my opinion. 
This will enable it on all non-Darwin systems, which should include PWR7 and PWR8, regardless of endianness. I'm happy to change this if there is something more specific to use. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:143
+    if (!thisBlockISELs.empty())
+      ISELInstructions.insert(std::make_pair(MBB.getNumber(), thisBlockISELs));
+  }
----------------
nemanjai wrote:
> Is there any concern that there may be some [pathological] cases where this would end up copying an unreasonably large vector (i.e. if the BB contains many isel instructions and has heap-allocated storage)?
You could certainly construct such a case, but I doubt it would happen in practice.



================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:182
+
+  assert((Opcode == PPC::ISEL || Opcode == PPC::ISEL8) &&
+         "Expecting an ISEL instruction");
----------------
nemanjai wrote:
> Not that this is likely to ever change, but perhaps for the sake of consistency this assert should be:
> ```
> assert(isISEL(MI) && "Expecting an ISEL instruction");
> ```
Good call, done. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:222
+    assert(MBB->canFallThrough() && "This block should fall through!\n");
+    for (MachineBasicBlock::succ_iterator succ_it = MBB->succ_begin();
+         succ_it != MBB->succ_end(); succ_it++) {
----------------
nemanjai wrote:
> Minor nit: I think that "succ_it" (although a cool variable name :)) breaks with variable naming conventions (camel vs. underscores).
Fixed


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:267
+  // MachineInstr *i1=
+  BuildMI(*TrueBlock, TrueBlockI, dl, TII->get(PPC::ADDI8))
+      .addOperand(Dest)
----------------
nemanjai wrote:
> I think for both cases where you add this instruction, a check should be performed whether the two registers are the same. If they are, you don't need the instruction. Also, just out of curiosity, is there any significance to the choice to use ADDI vs. OR (which I think is what we normally use for reg-to-reg copies)?
OR treats R0 as special (the literal value 0), while ADDI respects the contents of R0. So, if the register allocator has decided to use R0 for one of the inputs to ISEL, and you simply do an OR, you can end up with incorrect results. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:271
+      .addOperand(MachineOperand::CreateImm(0))
+      .getInstr();
+
----------------
nemanjai wrote:
> This is probably not needed since you're not saving a pointer to the instruction. Same applies below.
Good point, fixed.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:284
+
+  // Conditional branch to the TrueBlock
+  // MachineInstr *i2 =
----------------
nemanjai wrote:
> I wonder if there's anything available at this stage in the compilation that can be used for some heuristics to choose which block we should branch to and which should be a fall-through block?
Yes, the expansion can certainly be changed. We can discuss various heuristics, if we decide we want to actually expand the ISELs. For now I'd prefer to leave this as the default until that decision has been made. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:129
+
+bool PPCExpandISEL::isExpandISELEnabled(const MachineFunction &MF) {
+  if (GenerateISEL)
----------------
nemanjai wrote:
> 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.
Yes, I agree with this given the new direction of the patch (continue to generate ISEL, provide a switch to disable it).
If we want to disable ISEL generation by default, we can assess whether we should add similar logic to focus it to a subset of architectures. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:302
+    NewSuccessor->addLiveIn(TrueValue.getReg());
+    NewSuccessor->addLiveIn(FalseValue.getReg());
+  }
----------------
nemanjai wrote:
> One of these may be redundant. I think if the code is reorganized to keep things together, it will be clearer what needs to be done. What I mean is that if we group together things we do if we need a true block and group together things we do if we need a false block, the logic will be easier to follow.
I agree - I think we should be able to consolidate some of this code into just two tests.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:199
+  if (!IsTrueBlockRequired && !IsFalseBlockRequired) {
+    DEBUG(dbgs() << "Remove redudant ISEL instruction.");
+    MI.eraseFromParent(); // remove the ISEL instruction
----------------
Could you please add a statistic to track the number of ISEL instructions that are removed. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:225
+  // Special case 2, the two input registers used by ISEL are the same.
+  if (TrueValue.getReg() == FalseValue.getReg()) {
+    DEBUG(dbgs() << "Fold the ISEL insturction to an unconditonal copy.");
----------------
This should be done earlier, especially before (possibly) creating the NewSuccessor basic block above. 
I would move this as early as possible, once all the information needed is available. 


================
Comment at: test/CodeGen/PowerPC/expand-isel.ll:1
+; RUN: llc -verify-machineinstrs -O2 -ppc-asm-full-reg-names -ppc-gen-isel=false < %s | FileCheck %s -implicit-check-not isel
+; RUN: llc -verify-machineinstrs -O2 -ppc-asm-full-reg-names -ppc-gen-isel=false < %s | FileCheck %s -implicit-check-not isel
----------------
You need to include PowerPC triple


https://reviews.llvm.org/D23630