[PATCH] D46695: [RFC] [Patch 1/3] Add a new class of predicates for variant scheduling classes.

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: include/llvm/Target/TargetInstrPredicate.td:110
+// operand at position `Index` is a register operand.
+class CheckRegOperandValue<int Index, Register R>
+    : MCOperandPredicate<Index> {
----------------
andreadb wrote:
> rengolin wrote:
> > RKSimon wrote:
> > > rengolin wrote:
> > > > andreadb wrote:
> > > > > RKSimon wrote:
> > > > > > The use of 'Value' makes is it sound like we need the register to contain a certain value, not that it must be a certain register.
> > > > > I am okay with bikeshedding names if people don't like them.
> > > > > 
> > > > > What if I change `CheckRegOperand` into `CheckOperandIsRegister`, and then rename `CheckRegOperandValue` to `CheckRegOperand`?
> > > > This would make git archaeology confusing... :) 
> > > @rengolin Do you mean if the names have to change in the future after the patch is committed? A big concern is tablegen's lack of scoping might cause clashes in the future.
> > No, the proposal seem to be A -> B, C -> A; which will make confusing to compare patches on the two sides of the change regarding `CheckRegOperand` and what it means, but that's probably a minor concern.
> > 
> > Anyhow, I'm not trying to chime in on a bikeshedding argument. :)
> If you mean D46697, then that patch is no longer valid. I am going to close it (or update it to reflect these last changes).
Right, sounds good, sorry for the noise.


https://reviews.llvm.org/D46695