- 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.
Details
- Reviewers
arsenm bogner rampitec - Commits
- rGb1360caa823d: [SDAG] Add new AssertAlign ISD node.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
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? |
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp | ||
---|---|---|
138 | That's a hidden option disabling AssertAlign inserting to make the debug of regressions easier. |
LGTM minus the random formatting changes
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp | ||
---|---|---|
1723–1737 | Lots of unrelated formatting changes? |
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. |
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp | ||
---|---|---|
1723–1737 | Could switch to early return and avoid it? |
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. |
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp | ||
---|---|---|
1723–1737 | Duplicating the trivial case isn't a big deal |
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.