[PATCH] D46662: [X86] condition branches folding for three-way conditional codes

[Rong Xu via Phabricator via llvm-commits]

xur marked 6 inline comments as done.
xur added a comment.

Thanks Andrea's more recently review and very helpful suggestions. Here is the updated patch.



================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:90-101
+struct TargetMBBInfo {
+  MachineBasicBlock &MBB;
+  MachineBasicBlock *TBB;
+  MachineBasicBlock *FBB;
+  MachineInstr *BrInstr;
+  MachineInstr *CmpInstr;
+  X86::CondCode BranchCode;
----------------
andreadb wrote:
> Is field `MBB` ever used?
> 
> If not, then you can remove it.
> It is quite a big auxiliary struct. I wonder if it could be smaller... 
removed MBB field.

the main reason for so many fields is to reuse the analysis information in optimization phrase. 
we can reduce the struct size but we need to recompute.





================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:131-137
+  TargetMBBInfo *getMBBInfo(MachineBasicBlock *MBB) const {
+    assert(MBB);
+    int Num = MBB->getNumber();
+    if (!MBBInfos[Num])
+      return nullptr;
+    return MBBInfos[Num].get();
+  }
----------------
andreadb wrote:
> MBBInfos is initialized with one element (a descriptors) per each machine basic block. 
> If we don't create new basic blocks, then this code could just be rewritten as:
> ```
> return MBBInfos[MBB->getNumber()].get();
> ```
> 
> About the assert. If I understand your algorithm corectly, getMBBInfo is never called on a null MBB.
> That being said, it is not wrong to have an assert that validates a precondition. If you want to keep it, then please add a meaningful message to it.
You are absolutely right on this.  I simplified the code.


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:149
+  int CmpValue = MBBInfo->CmpValue;
+  assert(MBBInfo);
+
----------------
andreadb wrote:
> This assert is completely redundant.
> If MBBInfo was null, then you would have already had a segfault at line 148 ...
> 
> (Also: please add string messages to asserts..)
I meant to have the assert right after getMBBInfo() call, as getMBBInfo can return nullptr. Fixed


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:398-400
+  // Setup data structures.
+  for (unsigned I = 0; I < MF.getNumBlockIDs(); I++)
+    MBBInfos.push_back(nullptr);
----------------
andreadb wrote:
> Replace this loop with:
> 
> ```
> MBBInfos.resize(MF.getNumBLockIDs());
> ```
> 
> I am not even sure that you actually need this loop.
> The loop at line 401 is essentially initializing MBBInfos. So, you could just merge these two loops, and have a single loop calls to `MBBInfos.emplace_back(..);`.
Indeed, resize() is better here.

I don't think we can merge two loops, because the iteration order of the loop are different. 


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:474-475
+  SrcReg = MI.getOperand(SrcRegIndex).getReg();
+  if (!MI.getOperand(ValueIndex).isImm())
+    return false;
+  CmpValue = MI.getOperand(ValueIndex).getImm();
----------------
andreadb wrote:
> Can this ever happen with those opcodes?
> I think you can safely convert this check into an assert.
you are right!
replaced with assert.


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:493
+  MachineInstr *CmpInstr;
+  X86::CondCode BranchCode;
+  unsigned SrcReg;
----------------
andreadb wrote:
> Is this variable really needed? It seems like you only use CC.
Good catch. This is a leftover of the refactoring.


https://reviews.llvm.org/D46662