This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner][NFC] Factor out ByteProvider
ClosedPublic

Authored by jrbyrnes on Jan 31 2023, 2:30 PM.

Details

Summary

Bring ByteProvider into header to serve as generic utility.

Diff Detail

Unit TestsFailed

Event Timeline

jrbyrnes created this revision.Jan 31 2023, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 2:30 PM
jrbyrnes requested review of this revision.Jan 31 2023, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 2:30 PM
jrbyrnes updated this revision to Diff 493741.Jan 31 2023, 2:41 PM

Fix comments.

jrbyrnes updated this revision to Diff 494442.Feb 2 2023, 3:05 PM

Restructure

jrbyrnes added a subscriber: arsenm.Feb 2 2023, 3:05 PM

@arsenm is that what you had in mind?

jrbyrnes updated this revision to Diff 495567.Feb 7 2023, 8:47 AM

Format + Comments

Factor out common code for use in https://reviews.llvm.org/D142782 . Template class for eventual porting to GlobalISel.

jrbyrnes updated this revision to Diff 499912.Feb 23 2023, 10:36 AM

Require contained class in ByteProvider to have getOpcode (class must be node in selection DAG)

tschuett added inline comments.
llvm/include/llvm/ADT/ByteProvider.h
27

Why struct, when you use private and public?

30

using instead of typedef?

52

Why struct?

67

Newline missing

jrbyrnes updated this revision to Diff 500262.Feb 24 2023, 11:19 AM
jrbyrnes marked 4 inline comments as done.

Address review comments -- thanks @tschuett

Add outside reviewers

arsenm added inline comments.Mar 17 2023, 9:46 AM
llvm/include/llvm/ADT/ByteProvider.h
54

probably should use int64_t offsets?

jrbyrnes updated this revision to Diff 506212.Mar 17 2023, 3:34 PM
jrbyrnes marked an inline comment as done.

int64_t offsets

RKSimon added inline comments.Apr 4 2023, 5:07 AM
llvm/include/llvm/ADT/ByteProvider.h
24

Is there anyway that we can avoid an anonymous namespace in a header like this?

jrbyrnes updated this revision to Diff 520790.May 9 2023, 12:25 PM
jrbyrnes marked an inline comment as done.

Rebase + move is_op into ByteProvider. Thanks @RKSimon for taking a look.

barannikov88 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
21

(Just being skeptical.)
IMHO the implementation is not that abstract to be put into ADT.
Comments and names suggest that it has relation to instruction selection.
Keeping it to SelectionDAG/TargetLowering/CodeGen would make the SFINAE redundant, too.
Is it going to be used for anything other than SDValue anyway?

jrbyrnes updated this revision to Diff 520858.May 9 2023, 4:25 PM

Hi @barannikov88 -- thanks for your comments. I tend to agree that it is not "abstract enough" for ADT as it is basically an ISel util -- I have moved it into include/llvm/CodeGen

That said, I think it should be generic in that it should be agnostic between SDNode and SDValu -- DAGCombiner currently uses SDNode but I am working on a patch which calculates ByteProviders in terms of SDValues https://reviews.llvm.org/D142782 . Eventually we will want to extend this work to GlobalISel as well, so it seems we may also want MachineInstrs as the Src.

RKSimon added inline comments.May 13 2023, 4:08 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7822–7823

Is there anywhere we can safely put a typedef to avoid carrying around the same template argument?

jrbyrnes updated this revision to Diff 523107.May 17 2023, 10:39 AM
jrbyrnes marked an inline comment as done.

Typdef SDNode *

tschuett added inline comments.May 17 2023, 10:41 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7822–7823

using SDByteProvider = ByteProvider<SDNode*>;

jrbyrnes updated this revision to Diff 528971.Jun 6 2023, 11:55 AM
jrbyrnes marked an inline comment as done.

Use SDByteProvider

Thanks all for comments thus far -- are there any objections to merging this?

arsenm added a comment.Jun 8 2023, 4:52 PM

I'd agree include/llvm/CodeGen would be better place than ADT

RKSimon added inline comments.Jun 15 2023, 11:24 AM
llvm/include/llvm/CodeGen/ByteProvider.h
1 ↗(On Diff #528971)

llvm/CodeGen/ByteProvider.h

18 ↗(On Diff #528971)

Fix header guards

RKSimon added inline comments.Jun 15 2023, 11:28 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8245

(style) missing assert message (or merge the 2 asserts):

assert(P.hasSrc() && isa<LoadSDNode>(P.Src.value()) && "Must be a memory byte provider");
8246

auto*

8283

(style) missing assert message (or merge the 2 asserts):

assert(P->hasSrc() && isa<LoadSDNode>(P->Src.value()) && "provenance should either be memory or zero");
8284

auto*

8365–8366

this assert isn't necessary as the cast<> will also assert (same for the other above which I suggested merging with the other assert....)

8366

auto *

jrbyrnes updated this revision to Diff 531858.Jun 15 2023, 11:49 AM
jrbyrnes marked 8 inline comments as done.

Thanks @RKSimon

LGTM with one minor - does anyone else have any comments?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8366

auto *FirstLoad

jrbyrnes updated this revision to Diff 531875.Jun 15 2023, 12:23 PM
jrbyrnes marked an inline comment as done.

Nit

RKSimon added inline comments.Jun 16 2023, 2:48 AM
llvm/include/llvm/CodeGen/ByteProvider.h
1 ↗(On Diff #528971)

Missing include path

jrbyrnes updated this revision to Diff 532674.Jun 19 2023, 8:24 AM

Include path

arsenm accepted this revision.Jun 19 2023, 8:27 AM
This revision is now accepted and ready to land.Jun 19 2023, 8:27 AM
This revision was landed with ongoing or failed builds.Jun 19 2023, 8:55 AM
This revision was automatically updated to reflect the committed changes.