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

[Lei Huang via Phabricator via llvm-commits]

lei added inline comments.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:295
 
+bool PPCInstrInfo::isReallyTriviallyReMaterializable(const MachineInstr &MI,
+                                                     AliasAnalysis *AA) const {
----------------
nemanjai wrote:
> MatzeB wrote:
> > echristo wrote:
> > > 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.
> > Indeed that is one of the many things in CodeGen that are really confusing (and unnecessarily so it seems). However stating it is deprecated will only add to to the confusion unless there are concrete plans to do something about it.
> > 
> > And I'd certainly welcome a change that removes the MCInstrDesc bit and only uses the callback and measures that it is indeed negligible in terms of compiletime.
> I would imagine that the comment about the deprecation of the flag comes right out of `include/llvm/CodeGen/MachineInstr.h`:
> 
> ```
> This flag is deprecated, please don't use it anymore.  If this
> flag is set, the isReallyTriviallyReMaterializable() method is called to
> verify the instruction is really rematable.
> ```
Yes. That is where I got the impression this flag is deprecated.


https://reviews.llvm.org/D34255