Page MenuHomePhabricator

[Alignment][NFC] Add DebugStr and operator*
ClosedPublic

Authored by gchatelet on Apr 3 2020, 6:56 AM.

Details

Summary

Also updates files to use them.

This is patch is part of a series to introduce an Alignment type.
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html
See this patch for the introduction of the type: https://reviews.llvm.org/D64790

Diff Detail

Unit TestsFailed

TimeTest
820 msClang.CodeGen::builtin-align-assumption.c
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -triple=x86_64-unknown-unknown /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGen/builtin-align-assumption.c -emit-llvm -O1 -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGen/builtin-align-assumption.c
20 msLLVM.CodeGen/SystemZ::alloca-04.ll
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/llc < /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/SystemZ/alloca-04.ll -mtriple=s390x-linux-gnu -debug-only=codegen 2>&1 | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/SystemZ/alloca-04.ll
1,430 msLLVM.Transforms/AlignmentFromAssumptions::amdgpu-crash.ll
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/opt < /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/Transforms/AlignmentFromAssumptions/amdgpu-crash.ll -alignment-from-assumptions -S
1,550 msLLVM.Transforms/AlignmentFromAssumptions::simple.ll
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/opt < /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/Transforms/AlignmentFromAssumptions/simple.ll -alignment-from-assumptions -S | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/Transforms/AlignmentFromAssumptions/simple.ll
1,350 msLLVM.Transforms/AlignmentFromAssumptions::simple32.ll
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/opt < /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/Transforms/AlignmentFromAssumptions/simple32.ll -alignment-from-assumptions -S | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/Transforms/AlignmentFromAssumptions/simple32.ll

Event Timeline

gchatelet created this revision.Apr 3 2020, 6:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 6:57 AM
courbet added inline comments.Apr 3 2020, 7:19 AM
llvm/include/llvm/Support/Alignment.h
398

What is the meaning of Align(1) * 3 ?

courbet added inline comments.Apr 3 2020, 7:20 AM
llvm/include/llvm/Support/Alignment.h
398

My point is, I think what you want is an operator<<()

gchatelet marked an inline comment as done.Apr 3 2020, 7:39 AM
gchatelet added inline comments.
llvm/include/llvm/Support/Alignment.h
398

Since we return an Align (or MaybeAlign) the result have to be a power of two (or it will assert).
I can make it clear by requesting a power of two for Rhs.

I agree that a shift operator has the correct semantic and is less error prone but then the call site would be harder to understand from

MTI->setDestAlignment(I.getDestAlign() * DFSF.DFS.ShadowWidthBytes);
MTI->setSourceAlignment(I.getSourceAlign() * DFSF.DFS.ShadowWidthBytes);

to

MTI->setDestAlignment(I.getDestAlign() << Log2_64(DFSF.DFS.ShadowWidthBytes));
MTI->setSourceAlignment(I.getSourceAlign() << Log2_64(DFSF.DFS.ShadowWidthBytes));

WDYT?

courbet accepted this revision.Apr 5 2020, 11:26 PM
courbet added inline comments.
llvm/include/llvm/Support/Alignment.h
398

Alright, makes sense.

This revision is now accepted and ready to land.Apr 5 2020, 11:26 PM
This revision was automatically updated to reflect the committed changes.
gchatelet marked an inline comment as done.