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

Matthias Braun via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 20 10:25:16 PDT 2017


MatzeB added inline comments.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:295
 
+bool PPCInstrInfo::isReallyTriviallyReMaterializable(const MachineInstr &MI,
+                                                     AliasAnalysis *AA) const {
----------------
lei wrote:
> lei wrote:
> > 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.
> I'll add a comment here, but a detailed description can also be found in  `include/llvm/Target/TargetInstrInfo.h`
> 
> ```
>   /// For instructions with opcodes for which the M_REMATERIALIZABLE flag is
>   /// set, this hook lets the target specify whether the instruction is actually
>   /// trivially rematerializable, taking into consideration its operands. This
>   /// predicate must return false if the instruction has any side effects other
>   /// than producing a value, or if it requres any address registers that are
>   /// not always available.
>   /// Requirements must be check as stated in isTriviallyReMaterializable() .
>   virtual bool isReallyTriviallyReMaterializable(const MachineInstr &MI,
>                                                  AliasAnalysis *AA) const {
>     return false;
>   }
> ```
> I would imagine that the comment about the deprecation of the flag comes right out of include/llvm/CodeGen/MachineInstr.h:

Ah, I was looking at `include/llvm/Target/Target.td` for the definition/explanation of the instruction flags on the tablegen side (because at the point in `MCInstrDesc.h` where they are defined we have no further explanation). Odd that we only had the deprecation information on the convenience function in MachineInstr.


https://reviews.llvm.org/D34255





More information about the llvm-commits mailing list