llvm dev - Mar 2018 - [RFC] Function pointer alignment

Hello, all. Recently we noticed a problem with function pointer alignment in
LLVM: the
value tracking analysis (specifically the 'computeKnownBits' function)
uses function
alignment to infer that the least significant bits of function pointers are
known to be
zero. Unfortunately, this is not correct for ARM targets: the least significant
bit of a
function pointer stores the ARM/Thumb state information (i.e., the LSB is set
for Thumb
functions and cleared for ARM functions). The problem can be illustrated by the
following
test case:

; RUN: opt -instcombine -S < %s | FileCheck %s

target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"

; CHECK-LABEL: @foo_ptr
; CHECK: and
define i32 @foo_ptr() {
entry:
   ret i32 and (i32 ptrtoint (void ()* @foo to i32), i32 -4)
}

define internal void @foo() align 16 {
entry:
   ret void
}

LLVM eliminates the 'and' instruction because it incorrectly infers that
the two least
significant bits of the 'ptrtoint' result are zero.
I think that this can be fixed by adding a new field to the llvm::DataLayout
structure.
What do you think about such approach?

-- 
Regards,
   Mikhail Maltsev

On 03/20/2018 09:24 AM, Mikhail Maltsev via llvm-dev
wrote:
> Hello, all. Recently we noticed a problem with function pointer
> alignment in LLVM: the value tracking analysis (specifically the
> 'computeKnownBits' function) uses function alignment to infer that
the
> least significant bits of function pointers are known to be zero.
> Unfortunately, this is not correct for ARM targets: the least
> significant bit of a function pointer stores the ARM/Thumb state
> information (i.e., the LSB is set for Thumb functions and cleared for
> ARM functions). The problem can be illustrated by the following test
> case:
>
> ; RUN: opt -instcombine -S < %s | FileCheck %s
>
> target datalayout =
"e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
>
> ; CHECK-LABEL: @foo_ptr
> ; CHECK: and
> define i32 @foo_ptr() {
> entry:
>   ret i32 and (i32 ptrtoint (void ()* @foo to i32), i32 -4)
> }
>
> define internal void @foo() align 16 {
> entry:
>   ret void
> }
>
> LLVM eliminates the 'and' instruction because it incorrectly infers
> that the two least significant bits of the 'ptrtoint' result are
zero.
> I think that this can be fixed by adding a new field to the
> llvm::DataLayout structure. What do you think about such approach?
This makes sense to me. Right now, we assume that we can get the ABI
pointer alignment given only the address space of the pointer. Please
make sure you audit all uses of getPointerABIAlignment (and others as
necessary) to make sure that everything is updated consistently

 -Hal

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory