Page MenuHomePhabricator

[GlobalISel][TableGen] Add BitsPerByte value
AbandonedPublic

Authored by ehjogab on Dec 28 2020, 6:44 AM.

Details

Summary

In some targets, the smallest addressable unit does not consist of 8
bits. For such targets, the matcher code for G_SEXTLOAD and G_ZEXTLOAD
is incorrect and never yield a match. This patch addresses this problem
by introducing a new value BitsPerByte, thereby allowing the 8-bit
assumption to be removed from TableGen.

Change-Id: Ic6a3e6976a960c8ef65b4a7dbb065b2529d07dd9

Diff Detail

Event Timeline

ehjogab created this revision.Dec 28 2020, 6:44 AM
ehjogab requested review of this revision.Dec 28 2020, 6:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 28 2020, 6:44 AM
Herald added a subscriber: wdng. · View Herald Transcript
llvm/utils/TableGen/GlobalISelEmitter.cpp
3814–3815

Could you fix the English in the comment above?

I should know, but which getSizeInBits() is this calling? There are two definitions, both with the 8 hardwired. One is in CodeGen/MachineMemOperand.h and the other in ir/DataLayout.h.

Ah, is it calling the one in LowLevelTypeImpl.h? That one doesn't have a hardwired 8, although the function getSizeInBytes does.

ehjogab updated this revision to Diff 313938.Dec 29 2020, 12:05 AM

Fix lint warnings
Add testcase

ehjogab marked an inline comment as done.Dec 29 2020, 12:10 AM

I believe it is calling the one in LowLevelTypeImpl.h.

I agree that things become confusing when using hardcoded 8s in some places and then not in other places. Preferably, all hardcoded 8s should be replaced by something akin to this; we have done this in the codebase for our target but been unable to upstream the patch (for reasons unknown to me).

lebedev.ri requested changes to this revision.Dec 29 2020, 12:10 AM
lebedev.ri added a reviewer: jfb.
lebedev.ri added a subscriber: lebedev.ri.

This should be an RFC to llvm-dev, because this tries to silently lift the restriction that byte is 8 bits,
which is prevalent through llvm. And i do mean silently, because this has been brought up before a number of times.

This revision now requires changes to proceed.Dec 29 2020, 12:10 AM
ehjogab abandoned this revision.Jan 19 2021, 10:30 PM