[PATCH] D34255: [PowerPC] define target hook isReallyTriviallyReMaterializable

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:295
 
+bool PPCInstrInfo::isReallyTriviallyReMaterializable(const MachineInstr &MI,
+                                                     AliasAnalysis *AA) const {
----------------
lei wrote:
> echristo wrote:
> > This needs a comment as to why each of these need to be marked as rematerializable here rather than in the .td file.
> Will add:
> 
> // The Rematerializable flag is deprecated and if it is set, isReallyTriviallyReMaterializable() method need to be called to verify the instruction is really rematable.
Why do you get the impression that this flag is deprecated? It's just not the only thing checked as far as I can see.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:299
+  default: 
+    llvm_unreachable("Can not confirm opcode is really rematerializable");
+    break;
----------------
echristo wrote:
> I'm not sure an unreachable is the right thing here - usually this is reserved for switch cases that can't be true rather than just a return false.
This really should only be called for opcodes with the ReMaterializable flag set, so unreachable seems correct to me.


https://reviews.llvm.org/D34255