[PATCH] D70651: [Power8] Add the MacroFusion support for Power8

[qshanz via Phabricator via llvm-commits]

steven.zhang marked an inline comment as done.
steven.zhang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:136
+  llvm_unreachable("All the cases should have been handled");
+  return true;
+}
----------------
steven.zhang wrote:
> kbarton wrote:
> > steven.zhang wrote:
> > > kbarton wrote:
> > > > From reading the code, it seems like returning true from this function means that they should be scheduled together.
> > > > Thus, this function defaults to fusing instructions if it comes across something it doesn't understand (and asserts are not enabled). Would it not be better to return false here, and let the existing scheduling heuristics decide whether to make the instructions adjacent?
> > > When the checkOpConstraints() is called, we have got the fusion pair(FirstMI and SecondMI are fusion pair according to opcode). What we need to do is to kick some situation that we are sure that, it is NOT a fusion pair according to the spec. So, we have to return true by default unless we have the reason(i.e. the switch case) to say, it is NOT a fusion pair, as this is the last check. 
> > Could you please explain this in the comments to the function? 
> > 
> > Also, is it possible to add an assert at the checking that FirstMI and SecondMI are a valid fusion pair? This will validate your assumption, and also prevent the function from being used erroneously in the future. 
> ok, I will do it.
I have added the comments in the default case to explain why it returns true. Regarding to the assertion you mentioned, it is not easy to add it as all other conditions are within shouldScheduleAdjacent() and we are called by it also. I think, as this is a static function and we have the context of the call, it is not a big issue if we didn't add the assertion. This is the context of this call:
```
shouldScheduleAdjacent() {

 foreach fusion candidates {
   if (cannot fuse)
       continue
    
    if (checkOpConstraints(...))
        return true;
}
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70651/new/

https://reviews.llvm.org/D70651