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

[Kit Barton via Phabricator via llvm-commits]

kbarton added a comment.

Could you please rebase this patch?
It no longer applies cleanly, since you committed https://reviews.llvm.org/D70768.



================
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:
> > 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. 


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

https://reviews.llvm.org/D70651