[PATCH] D86215: [TableGen][GlobalISel] Fix handling of zero_reg

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:2656
+      Table << MatchTable::Comment("zero_reg");
+      Table << MatchTable::IntValue(0);
+    }
----------------
ehjogab wrote:
> arsenm wrote:
> > ehjogab wrote:
> > > bjope wrote:
> > > > ehjogab wrote:
> > > > > bjope wrote:
> > > > > > arsenm wrote:
> > > > > > > ehjogab wrote:
> > > > > > > > arsenm wrote:
> > > > > > > > > Could this use TargetNamespace::NoRegister?
> > > > > > > > I suppose that works. I checked for our target and NoRegister is indeed assigned value 0, so the semantics would be the same. But I am not sure whether that would work for all targets. If it does, I do believe your suggestion to be a better solution than to simply output value 0. Perhaps someone more knowledgeable regarding TableGen could comment on this?
> > > > > > > That's generated for all targets. 0 is always $noreg, with an added NoRegister entry.
> > > > > > > 
> > > > > > > I also think zero_reg should really be renamed to $noreg or something closer to what it really is
> > > > > > Agree `zero_reg` is not a good name if this refers to `NoRegister` (register number zero in the internal representation).
> > > > > > 
> > > > > > I would have guessed that it mapped to a register always being read as zero (if the target got such a register).
> > > > > > 
> > > > > > Changing the name is perhaps outside the scope of this patch. But we better (make sure) clarify that zero_reg is NoRegister (holding an undefined value, rather than holding the value zero).
> > > > > Looking into the Target/Target.td file where zero_reg is defined, it says the following:
> > > > > 
> > > > > ```
> > > > > /// zero_reg definition - Special node to stand for the zero register.
> > > > > ///
> > > > > def zero_reg;
> > > > > ```
> > > > > 
> > > > > Is "zero register" and "NoRegister" really the same thing? If not, what should then be output?
> > > > MIRLangRef also talks about "The null registers".
> > > > 
> > > > I think that `TargetNamespace::NoRegister`, `zero_reg`, `null registers`, `_` (in MIR and dumps) and `$noreg` more or less all refer to the same thing - register number zero. But perhaps some targets could map a register always being read as zero to `$noreg`, e.g. translating that specifier to register number 31 in the asm printer.
> > > > 
> > > > It is confusing to have that many different names for the same thing. And to me `zero_reg` and `null register` is most confusing as there are targets that have registers always being read as zero (and the register number for such a register could be any number). So unless `zero_reg` actually is supposed to refer to a register being read as zero, it would be better to for  example call it `noreg` or `reg_null`.
> > > > 
> > > > Looks like SystemZ got one use of `zero_reg`, and the rest belongs to ARM.
> > > Then I suppose we will continue outputting "NoRegister"; I just need to figure out how to retrieve the "NameSpace" from the zero_reg directive, since that's not immediately available...
> > Actually I think the code would work as is for this. If you look at CopyOrAddZeroRegRenderer,
> > 
> >          << MatchTable::NamedValue(
> >                  (ZeroRegisterDef->getValue("Namespace")
> >                       ? ZeroRegisterDef->getValueAsString("Namespace")
> >                       : ""),
> I have looked into that but RegisterDef does not have "Namespace" in the case of zero_reg, so I need to retrieve the namespace information elsewhere.
So this check for the namespace is pointless and should be removed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86215/new/

https://reviews.llvm.org/D86215