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

[Tony Jiang via Phabricator via llvm-commits]

jtony marked 9 inline comments as done.
jtony added inline comments.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:267
+  // MachineInstr *i1=
+  BuildMI(*TrueBlock, TrueBlockI, dl, TII->get(PPC::ADDI8))
+      .addOperand(Dest)
----------------
kbarton wrote:
> 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. 
Nemanjaj already noticed the bug in the patch  :-)  (always use ADDI for both True and False branch, we should just use ADDI for True branch and ORI  for false branch), if we have considered this comments more carefully, could avoid this bug.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:33
+    GenerateISEL("ppc-gen-isel",
+                 cl::desc("Enable generating the ISEL instruction"),
+                 cl::init(true), cl::Hidden);
----------------
nemanjai wrote:
> Should probably be changed to something along the lines of
> `Disable generating the ISEL instruction on 64-bit non-Darwin PPC targets.`
I reconsider this comment and realize that this ppc-gen-isel option is set to true, maybe it is better (I mean consistent with the default behavior) we use the "Enable generating the ISEL instruction"?


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:34
+                 cl::desc("Enable generating the ISEL instruction"),
+                 cl::init(true), cl::Hidden);
+
----------------
Shall we make this option visible to all users?


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:302
+    NewSuccessor->addLiveIn(TrueValue.getReg());
+    NewSuccessor->addLiveIn(FalseValue.getReg());
+  }
----------------
kbarton wrote:
> 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.
Last time when I move around these code, it caused assertion, so I just combined a few blocks, leave some other blocks untouched. If people strongly disagree, I can try again to move these blocks around.


https://reviews.llvm.org/D23630