Page MenuHomePhabricator

[TableGen][GlobalISel] Fix handling of zero_reg
ClosedPublic

Authored by ehjogab on Aug 19 2020, 6:56 AM.

Details

Summary

When generating matching tables for GlobalISel, TableGen would output
"::zero_reg" whenever encountering the zero_reg, which in turn would
result in compilation error. This patch fixes that by instead outputting
NoRegister (== 0), which is the same result that TableGen produces when
generating matching tables for ISelDAG.

Diff Detail

Event Timeline

ehjogab created this revision.Aug 19 2020, 6:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 6:56 AM
Herald added subscribers: bjope, jfb, rovka. · View Herald Transcript
ehjogab requested review of this revision.Aug 19 2020, 6:56 AM
arsenm added inline comments.Aug 19 2020, 7:04 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

Could this use TargetNamespace::NoRegister?

ehjogab added inline comments.Aug 19 2020, 8:52 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

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?

arsenm added inline comments.Aug 19 2020, 9:41 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

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

bjope added inline comments.Aug 19 2020, 10:17 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

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).

ehjogab added inline comments.Aug 19 2020, 11:45 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

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?

bjope added inline comments.Aug 20 2020, 6:31 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

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.

ehjogab added inline comments.Aug 20 2020, 10:31 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

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...

arsenm added inline comments.Aug 21 2020, 6:42 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

Actually I think the code would work as is for this. If you look at CopyOrAddZeroRegRenderer,

<< MatchTable::NamedValue(
        (ZeroRegisterDef->getValue("Namespace")
             ? ZeroRegisterDef->getValueAsString("Namespace")
             : ""),
ehjogab added inline comments.Aug 24 2020, 2:06 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

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.

arsenm added inline comments.Aug 24 2020, 7:42 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

So this check for the namespace is pointless and should be removed?

ehjogab updated this revision to Diff 287401.Aug 24 2020, 8:12 AM
ehjogab marked an inline comment as not done.

[TableGen][GlobalISel] Fix handling of zero_reg

When generating matching tables for GlobalISel, TableGen would output
"::zero_reg" whenever encountering the zero_reg, which in turn would
result in compilation error. This patch fixes that by instead outputting
NoRegister (== 0), which is the same result that TableGen produces when
generating matching tables for ISelDAG.

ehjogab updated this revision to Diff 287402.Aug 24 2020, 8:14 AM
ehjogab marked an inline comment as not done.

Forgot to update testcase.

ehjogab added inline comments.Aug 24 2020, 8:18 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2656

In the case of ZeroRegisterDef it seems to be a different story, as ZeroRegisterDef takes the value of GIZeroRegister whenever it is used, which could be any kind of register which may or may not have a Namespace. In the case of zero_reg, however, it always does not have a Namespace.

I have now updated the patch to look over the defs present in the target in search of a valid Namespace, so now we can use NoRegister (although the fix for this feels hacky, to me).

bjope added inline comments.Aug 24 2020, 10:52 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
2670

Maybe one can add something like this to CodeGenTaget (putting the code close to the getInstNamespace support):

StringRef CodeGenTarget::getRegNamespace() const {
  auto &RegClasses = RegBank.getRegClasses();
  return RegClasses.size() > 0 ? RegClasses.front().Namespace : "";
}

and then use Target.getRegNamespace() to get it.

ehjogab updated this revision to Diff 287932.Aug 26 2020, 5:04 AM

Updated patch according to bjope's suggestion.

ehjogab edited the summary of this revision. (Show Details)Aug 26 2020, 5:06 AM
ehjogab added a reviewer: bjope.
arsenm added inline comments.Aug 26 2020, 5:47 AM
llvm/utils/TableGen/CodeGenTarget.cpp
549 ↗(On Diff #287932)

Is using a different namespace from the overall target actually supported? Can't you just take the namespace from CodeGenTarget directly?

bjope added inline comments.Aug 26 2020, 9:03 AM
llvm/utils/TableGen/CodeGenTarget.cpp
547 ↗(On Diff #287932)

Just like in the header file. I'd rather put this next to getInstNamespace. This would also show more clearly that we currently fetch the namespaces in different ways, and by that we perhaps support different namespaces for regs and instructions etc. I have no idea if that is used (or useful) in reality.

I figure that we at least wouldn't break anything as long as we follow what CodeGenRegisters is using when defining the enum with NoRegister, when fetching the getRegNamespace.

llvm/utils/TableGen/CodeGenTarget.h
194 ↗(On Diff #287932)

I'd put this next to getInstNamespace(). Or if for some reason it would be better to keep "inst" helpers and "reg" helpers separated, then I'd put it next to something related to registers such as getRegBank or getRegRegisterByName. But I can't see any logic in the current order of methods (it's even mixing public and private declarations), so that's why I'd got for keeping the namespace-related functions close to each other.

ehjogab updated this revision to Diff 288896.Aug 31 2020, 12:28 AM
ehjogab edited the summary of this revision. (Show Details)

Reordered functions as per bjope's feedback

arsenm accepted this revision.Sep 14 2020, 1:28 PM

LGTM

This revision is now accepted and ready to land.Sep 14 2020, 1:28 PM
This revision was landed with ongoing or failed builds.Sep 18 2020, 2:02 AM
This revision was automatically updated to reflect the committed changes.
bjope added a comment.Sep 18 2020, 2:03 AM

I landed this on behalf of Gabriel.