Page MenuHomePhabricator

[Clang] Pragma vectorize_width() implies vectorize(enable), take 3
AcceptedPublic

Authored by SjoerdMeijer on Oct 30 2019, 10:21 AM.

Details

Summary

This got reverted because given the following source:

void a() {
  #pragma clang loop vectorize(disable)
  for (;;)
    ;
}

it incorrectly enabled vectorisation and set metadata due to a logic error. With this fixed, we now imply vectorisation when:

  1. vectorisation is enabled, which means: VectorizeWidth > 1
  2. and don't want to add it when it is disabled or enabled, otherwise we would be incorrectly setting it or duplicating the metadata, respectively.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Oct 30 2019, 10:21 AM
Meinersbur requested changes to this revision.Oct 30 2019, 11:03 AM
Meinersbur added inline comments.
clang/lib/CodeGen/CGLoopInfo.cpp
272–277

[serious] Please handle the llvm.loop.vectorize.enable metadata in one place, i.e. where the other llvm.loop.vectorize.enable is handled. This introduces yet another mechanism when to add llvm.loop.vectorize.enable besides the one for IsVectorPredicateEnabled. Btw, with vectorize_predicate(enable) vectorize_width(2) this emits two llvm.loop.vectorize.enable.

Also, the changing relative order of llvm.loop.vectorize.enable to other metadata makes D69092 difficult.

clang/test/CodeGenCXX/pragma-loop-predicate.cpp
61 ↗(On Diff #227137)

unintentional change?

clang/test/CodeGenCXX/pragma-loop.cpp
161–177

As you might have noticed, updating FileCheck for metadata in multiple test cases is a lot of work since they influence each other. Why not adding the new test in separate files?

221

-NEXT should be unnecessary here. I'd even go towards CHECK-DAG since the order of the metadata is unimportant.

This revision now requires changes to proceed.Oct 30 2019, 11:03 AM

Thanks Michael!

  • moved the handling of vectorize.enable to 1 place,
  • that should have also sorted the relative ordering, and duplication of the metadata in some cases,
  • created a separate file for the new tests, and added some more tests (also making sure there are no duplicates).

Sorry for the delay, for some reason I did not see a notification for the update.

clang/lib/CodeGen/CGLoopInfo.cpp
297–301

[serious] Why not reusing the Args.push_back code from above? I think it is important vectorize_predicate and vectorize_width (and ever additional property we introduce in the future) the same way. IMHO everything else becomes more and more confusing.
I have the following in mind:

if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
      IsVectorPredicateEnabled || Attrs.VectorizeWidth > 1) {
    auto AttrVal = Attrs.VectorizeEnable != LoopAttributes::Disable;
    Args.push_back(..., ConstantInt::get(AttrVal));
}
SjoerdMeijer marked an inline comment as done.Wed, Nov 13, 9:04 AM
SjoerdMeijer added inline comments.
clang/lib/CodeGen/CGLoopInfo.cpp
297–301

No worries, and thanks for looking again! I was a bit reluctant to touch that piece of logic (and actually thought this if-elseif was not too bad and explicit in identifying the different cases), but yeah, what you suggest make sense, so will address this soon.

Thanks again for the suggestion; this indeed (hopefully) looks a lot neater now.

Meinersbur added inline comments.Mon, Nov 18, 1:40 PM
clang/lib/CodeGen/CGLoopInfo.cpp
297–301

Sounds like a typical case of "technical debt".

SjoerdMeijer marked an inline comment as done.Sat, Nov 23, 1:24 AM
SjoerdMeijer added inline comments.
clang/lib/CodeGen/CGLoopInfo.cpp
297–301

yep, but it's been addressed

This revision is now accepted and ready to land.Mon, Nov 25, 10:32 AM