Page MenuHomePhabricator

[IR][doc] Alignment is always set in memory for load/store/alloca/cmpxchg/atomicrmw.
ClosedPublic

Authored by gchatelet on Jan 26 2023, 6:09 AM.

Diff Detail

Unit TestsFailed

TimeTest
60,060 msx64 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 msx64 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

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.

gchatelet updated this revision to Diff 492673.Jan 27 2023, 2:07 AM
  • Address comments

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.

efriedma accepted this revision.Jan 27 2023, 10:01 AM
efriedma added subscribers: xgupta, nikic.

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
gchatelet updated this revision to Diff 493252.Jan 30 2023, 2:30 AM
  • Address comments

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?

I suppose this should go away as well right?

Yes, I think so.

  • rebase
  • remove handling of the 0 value for alloca's alignement
This revision was landed with ongoing or failed builds.Jan 31 2023, 12:36 AM
This revision was automatically updated to reflect the committed changes.