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

[Eric Christopher via Phabricator via llvm-commits]

echristo added inline comments.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:295
 
+bool PPCInstrInfo::isReallyTriviallyReMaterializable(const MachineInstr &MI,
+                                                     AliasAnalysis *AA) const {
----------------
MatzeB wrote:
> 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.
Yeah. I was wondering about that. Also, that's really confusing. The routine itself could use a comment update explaining what's going on? Or at least it wasn't horribly clear to me.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:299
+  default: 
+    llvm_unreachable("Can not confirm opcode is really rematerializable");
+    break;
----------------
MatzeB wrote:
> 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.
Ah. I missed that, OK then. Probably needs to be something like "unknown rematerializable operation" or something.


https://reviews.llvm.org/D34255