[llvm-commits] [patch][pr12251] Add a "range" metadata, use it to remove unnecessary ands

[Chris Lattner]

On Mar 20, 2012, at 1:43 PM, Rafael Espíndola wrote:
> With these changes, we compile
> 
> bool foo(bool *x) {
>  return *x;
> }
> 
> to
> 
> define zeroext i1 @_Z3fooPb(i8* nocapture %x) nounwind uwtable
> readonly optsize {
> entry:
>  %0 = load i8* %x, align 1, !tbaa !0, !range !3
>  %tobool = icmp ne i8 %0, 0
>  ret i1 %tobool
> }
> 
> Which is probably as good as it gets in the IL if we are going to load
> an i8 and return an i1. The generated assembly is
> 
> 
> 	cmpb	$0, (%rdi)
> 	setne	%al
> 	ret

Nice :)

> Other questions:
> * ) should clang produce this metadata at -O0?

No, it's for optimization purposes only, so it shouldn't go out at -O0.

> *) should it be documented in the language ref?

Yes.

> *) should the verifier check it?

Yes, that would be great.


On the clang side of things:

+  if (const EnumType *ET = dyn_cast<EnumType>(Ty))
+    return ET->getDecl()->getIntegerType()->isBooleanType();

It seems that this could be more aggressive: my understanding is that in C++ if you have an enum with 3 values, you know that it is only has 2 bits set.  It would be nice expose this info to the optimizer as well.

+    llvm::Type *Int8 = llvm::Type::getInt8Ty(C);

Int8Ty is in scope here, through CodeGenTypesCache, please use it.

+    llvm::MDNode *Ranges =
+      llvm::MDNode::get(C, static_cast<llvm::Value*>(Range));

Why is the static_cast needed?


On the LLVM patch:

- The lib/Analysis/InstructionSimplify.cpp patch looks independent, please split it out.


--- a/lib/Analysis/ValueTracking.cpp
+++ b/lib/Analysis/ValueTracking.cpp
@@ -20,6 +20,7 @@
...
+  case Instruction::Load:
+    computeMaskedBitsLoad(cast<LoadInst>(I)->getMetadata(LLVMContext::MD_range),
+                          Mask, KnownZero);

Given that most loads won't have metadata, a better pattern would be:

  if (MDNode *MD = cast<LoadInst>(I)->getMetadata(LLVMContext::MD_range))
    computeMaskedBitsLoad(MD, Mask, KnownZero)

computeMaskedBitsLoad also needs a doc comment.

It's hard for me to tell what the actual format of the MDNode is, a LangRef patch would be really helpful for that :)

Thanks for working on this Rafael!

-Chris