This is an archive of the discontinued LLVM Phabricator instance.

[PGO][PGSO] DAG.shouldOptForSize part.
ClosedPublic

Authored by hjyamauchi on Nov 11 2019, 11:02 AM.

Details

Summary

(Split of off D67120)

SelectionDAG::shouldOptForSize changes for profile guided size optimization.

Event Timeline

hjyamauchi created this revision.Nov 11 2019, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2019, 11:02 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Looks good to me. Added Sanjay and Simon for review.

MaskRay added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
29

clang-format the header order.

hjyamauchi marked an inline comment as done.

Address comment.

I don't know about the overall changes planned, but this seems fine.
One question though: what about targets other than x86? Don't we need to update them in any places they are using 'getMachineFunction().hasOptSize()' as we are for x86?

Cover non-x86 targets (which happens to affect AArch64 files only). PTAL.

In D70095#1746161, @yamauchi wrote:

Cover non-x86 targets (which happens to affect AArch64 files only). PTAL.

WebAssembly too?

class WebAssemblyDAGToDAGISel final : public SelectionDAGISel {
...
  bool ForCodeSize;
...
  ForCodeSize = MF.getFunction().hasOptSize();
spatel added inline comments.Nov 14 2019, 12:37 PM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
402 ↗(On Diff #229358)

If this was the only use of the "ForCodeSize" class variable, then it should be removed now?

hjyamauchi marked an inline comment as done.

Address comment.

hjyamauchi added inline comments.Nov 15 2019, 9:38 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
402 ↗(On Diff #229358)

True, done.

Does WebAssembly still need updating ?

llvm/include/llvm/CodeGen/SelectionDAG.h
241

initialize with = nullptr

hjyamauchi marked an inline comment as done.

Address comments.

Does WebAssembly still need updating ?

I somehow missed the WebAssembly comment. Thanks for reminding.

I think we can remove WebAssemblyDAGToDAGISel::ForCodeSize and the hasOptSize call.
Three reasons:
(1) ForCodeSize is not used,
(2) We can't call DAG.shouldOptForSize there because DAG isn't initialized at that point (if we want to query this at a block granularity, it wouldn't make sense to call it there to begin with.)
(3) Removing it would minimize confusions, I think, in case we want to implement size optimizations here for WebAssembly in the future (in which case, we could follow the other targets code and use DAG.shouldOptForSize.)
Of course, we could alternatively leave it as is, and no update needed.

spatel accepted this revision.Nov 21 2019, 11:40 AM
In D70095#1755505, @yamauchi wrote:

Does WebAssembly still need updating ?

I somehow missed the WebAssembly comment. Thanks for reminding.

I think we can remove WebAssemblyDAGToDAGISel::ForCodeSize and the hasOptSize call.
Three reasons:
(1) ForCodeSize is not used,
(2) We can't call DAG.shouldOptForSize there because DAG isn't initialized at that point (if we want to query this at a block granularity, it wouldn't make sense to call it there to begin with.)
(3) Removing it would minimize confusions, I think, in case we want to implement size optimizations here for WebAssembly in the future (in which case, we could follow the other targets code and use DAG.shouldOptForSize.)

LGTM.
I agree that removing unused code is good, but I've never looked at WebAssembly before, so let's see if anyone else has comments (wait a day to commit).

This revision is now accepted and ready to land.Nov 21 2019, 11:40 AM
RKSimon accepted this revision.Nov 21 2019, 12:29 PM

LGTM - as @spatel suggested either wait a day or split off the WebAssembly diff (it appears to be completely independent + superfluous) into a separate patch for review and then you can commit this immediately.

Thanks, I'll try the option of splitting out the WebAssembly change.

Split the WebAssembly change.

This revision was automatically updated to reflect the committed changes.