Details

Reviewers

efriedma
nikic

Commits

rGa544846c0b73: [IR][doc] Alignment is always set in memory for…

Summary

Context https://reviews.llvm.org/D142459#4081179

Diff Detail

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	Time	Test
	60,060 ms	x64 debian > MLIR.Examples/standalone::test.toy Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir -DLLVM_USE_LINKER=lld -DPython3_EXECUTABLE="/usr/bin/python3.9"
	60,040 ms	x64 debian > lit.lit::max-failures.py Exception during script execution: Traceback (most recent call last): File "/var/lib/buildkite-agent/builds/llvm-project/llvm/utils/lit/lit/worker.py", line 76, in _execute_test_handle_errors

Event Timeline

gchatelet created this revision.Jan 26 2023, 6:09 AM

Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 6:09 AM

Herald added a subscriber: jdoerfert. · View Herald Transcript

gchatelet requested review of this revision.Jan 26 2023, 6:09 AM

Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 6:09 AM

Herald added a subscriber: llvm-commits. · View Herald Transcript

Harbormaster completed remote builds in B210109: Diff 492422.Jan 26 2023, 7:15 AM

The phrasing isn't grammatically correct. I would say something like:

"The alignment is only optional when parsing textual IR; for in-memory IR, it is always present."

If you want to try to rearrange the text a bit, maybe put all discussion of the "optional-ness" of the alignment into a separate paragraph from the main definition of alignment.

Address comments

In D142625#4083421, @efriedma wrote:

The phrasing isn't grammatically correct. I would say something like:

"The alignment is only optional when parsing textual IR; for in-memory IR, it is always present."

If you want to try to rearrange the text a bit, maybe put all discussion of the "optional-ness" of the alignment into a separate paragraph from the main definition of alignment.

Thx for the suggestion. I've reworded a bit, let me know what you think.

Harbormaster completed remote builds in B210297: Diff 492673.Jan 27 2023, 2:50 AM

LGTM, but there's probably a merge conflict with D142633.

This revision is now accepted and ready to land.Jan 27 2023, 10:01 AM

Address comments

In D142625#4086606, @efriedma wrote:

LGTM, but there's probably a merge conflict with D142633.

Thx for the heads up.
I've updated the patch accordingly but there is still a reference to 0 for alloca. I suppose this should go away as well right?

gchatelet added a reviewer: nikic.Jan 30 2023, 2:34 AM

Herald added a subscriber: StephenFan. · View Herald TranscriptJan 30 2023, 2:34 AM

Harbormaster completed remote builds in B210732: Diff 493252.Jan 30 2023, 4:51 AM