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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb 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> {
----------------
rengolin wrote:
> 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.
No problem. In retrospect, I should have created patch 2 of 3 from the start.
To avoid confusion, I have just abandoned D46697 in favor of D47077.

I hope it helps.

Cheers,
Andrea


https://reviews.llvm.org/D46695