Page MenuHomePhabricator

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

Authored by yamauchi on Mon, Nov 11, 11:02 AM.

Details

Summary

(Split of off D67120)

SelectionDAG::shouldOptForSize changes for profile guided size optimization.

Diff Detail

Event Timeline

yamauchi created this revision.Mon, Nov 11, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 11, 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
30

clang-format the header order.

yamauchi updated this revision to Diff 228914.Tue, Nov 12, 10:00 AM
yamauchi 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?

yamauchi updated this revision to Diff 229358.Thu, Nov 14, 10:55 AM

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

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.Thu, Nov 14, 12:37 PM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
398

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

yamauchi updated this revision to Diff 229574.Fri, Nov 15, 9:38 AM
yamauchi marked an inline comment as done.

Address comment.

yamauchi added inline comments.Fri, Nov 15, 9:38 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
398

True, done.

Does WebAssembly still need updating ?

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

initialize with = nullptr

yamauchi updated this revision to Diff 230492.Thu, Nov 21, 10:54 AM
yamauchi 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.Thu, Nov 21, 11:40 AM

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.Thu, Nov 21, 11:40 AM
RKSimon accepted this revision.Thu, Nov 21, 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.

yamauchi updated this revision to Diff 230519.Thu, Nov 21, 1:16 PM

Split the WebAssembly change.

This revision was automatically updated to reflect the committed changes.