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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In https://reviews.llvm.org/D46662#1248780, @xur wrote:

> Using new SubtargetFeature method (suggested by Andrea) to make this pass opt-in for subtargets.
>  Changed the tests accordingly.


Thanks Rong.

I have some comments (See below).

Cheers
-Andrea



================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:90-101
+struct TargetMBBInfo {
+  MachineBasicBlock &MBB;
+  MachineBasicBlock *TBB;
+  MachineBasicBlock *FBB;
+  MachineInstr *BrInstr;
+  MachineInstr *CmpInstr;
+  X86::CondCode BranchCode;
----------------
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... 


================
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();
+  }
----------------
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.


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:149
+  int CmpValue = MBBInfo->CmpValue;
+  assert(MBBInfo);
+
----------------
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..)


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:398-400
+  // Setup data structures.
+  for (unsigned I = 0; I < MF.getNumBlockIDs(); I++)
+    MBBInfos.push_back(nullptr);
----------------
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(..);`.


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:446-447
+                                      int &CmpValue) {
+  int SrcRegIndex = 0;
+  int ValueIndex = 0;
+  switch (MI.getOpcode()) {
----------------
unsigned.


================
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();
----------------
Can this ever happen with those opcodes?
I think you can safely convert this check into an assert.


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:493
+  MachineInstr *CmpInstr;
+  X86::CondCode BranchCode;
+  unsigned SrcReg;
----------------
Is this variable really needed? It seems like you only use CC.


https://reviews.llvm.org/D46662