Page MenuHomePhabricator

[SDAG] Add new AssertAlign ISD node.
ClosedPublic

Authored by hliao on Jun 11 2020, 8:28 PM.

Details

Summary
  • AssertAlign node records the guaranteed alignment on its source node, where these alignments are retrieved from alignment attributes in LLVM IR. These tracked alignments could help DAG combining and lowering generating efficient code.
  • In this patch, the basic support of AssertAlign node is added. So far, we only generate AssertAlign nodes on return values from intrinsic calls.
  • Addressing selection in AMDGPU is revised accordingly to capture the new (base + offset) patterns.

Diff Detail

Event Timeline

hliao created this revision.Jun 11 2020, 8:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2020, 8:28 PM
craig.topper added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
3175

Known.One should already be all 0s. I don't think you need to clear it. Though maybe you should call computeKnownBits to propagate from the input? Then you would need to clear it.

hliao marked an inline comment as done.Jun 12 2020, 8:23 AM
hliao added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
3175

AssertAlign is intended to be used with align function attributes, which could be used to annotate alignment on return value or arguments. Most of them are lowered into register copies. It's difficult and almost no way to trace back the original alignment embedded in LLVM IR. AssertAlign is added to propagate the alignment info from IR into DAG. It helps computeKnowBits instead of relying on it.

hliao updated this revision to Diff 270562.Jun 12 2020, 8:56 PM

Rebase to trunk.

arsenm added inline comments.Jun 15 2020, 5:04 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5201–5204

This check is unnecessary, a lower alignment isn't even representable in Align because it stores the log2 of the alignment

5201–5204

Nevermind, I misread this. I think it would be clearer as if (A == Align(1)) return Val

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
138

Not sure we really need this?

hliao marked an inline comment as done.Jun 15 2020, 7:46 AM
hliao added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
138

That's a hidden option disabling AssertAlign inserting to make the debug of regressions easier.

arsenm added inline comments.Jun 17 2020, 5:17 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5201–5204

I think == Align(1) would be clearer

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1635

Can you add a dag style comment for the pattern this matches? This could also use some early returns

hliao updated this revision to Diff 271797.Jun 18 2020, 11:32 AM

Revise following reviewer's comments.

hliao marked 4 inline comments as done.Jun 18 2020, 11:34 AM

LGTM minus the random formatting changes

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1723–1737

Lots of unrelated formatting changes?

hliao marked an inline comment as done.Jun 18 2020, 2:02 PM
hliao added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1723–1737

That's due to the extra indent added from L1680. Also, the lint progress in arc review tries to re-formatting all changed code.

arsenm added inline comments.Jun 18 2020, 2:16 PM
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1723–1737

Could switch to early return and avoid it?

hliao marked an inline comment as done.Jun 18 2020, 5:36 PM
hliao added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1723–1737

That code just conditionally refine Addr and, eventually, all paths need to join L1760 to prepare all the return values. Unless goto is used or code duplication, early return cannot be used here.

ping for code review

arsenm accepted this revision.Jun 22 2020, 12:46 PM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1723–1737

Duplicating the trivial case isn't a big deal

This revision is now accepted and ready to land.Jun 22 2020, 12:46 PM
This revision was automatically updated to reflect the committed changes.